[BUG]: Added return of the original function result when tracer not defined #1268
Conversation
…efined - Added _transform_attributes method as this seemed to be throwing an exception
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
@beggers, have a look. I might be wrong, but this worked for me using local tracing (referring to |
| @@ -129,4 +139,4 @@ def add_attributes_to_current_span( | |||
| if not tracer: | |||
| return | |||
| span = trace.get_current_span() | |||
| span.set_attributes(_transform_attributes(attributes)) # type: ignore | |||
| span.set_attributes(_transform_attributes(attributes)) | |||
There was a problem hiding this comment.
Ah, this was sloppy on my part. The correct fix here is to simply remove the call to _transform_attributes.
The call to _transform_attributes is a holdover from when add_attributes_to_current_span accepted a Dict[str, Any], since OTel wants attribute values to be one of [str, bool, float, int]. @HammadB and I talked about this in code review and decided we should strongly type add_attributes_to_current_span so transformation is no longer necessary.
All that said: let's delete _transform_attributes and the call to it.
There was a problem hiding this comment.
mypy is still complaining:
Should I do this instead?:
span.set_attributes({k: cast(types.AttributeValue, v)
for k, v in attributes.items()})It seems that Otel expects AttributeValue so we might have to do the conversion.
Note: We can even widen the accepted values to include sequences of the ones you mentioned above:
AttributeValue = Union[
str,
bool,
int,
float,
Sequence[str],
Sequence[bool],
Sequence[int],
Sequence[float],
]There was a problem hiding this comment.
Ah, this is why I shouldn't review code on Sunday night ;)
I like your original method of solving this, or I'd be happy to cast to AttributeValue inline in add_attributes_to_current_span -- up to you. And sorry to have asked you to make an unnecessary change.
There was a problem hiding this comment.
Take a look at the latest change, it works and feels more minimalistic. (we live and die with mypy)
| @@ -110,7 +120,7 @@ def wrapper(*args: Any, **kwargs: Dict[Any, Any]) -> Any: | |||
| if trace_granularity < granularity: | |||
| return f(*args, **kwargs) | |||
| if not tracer: | |||
| return | |||
| return f(*args, **kwargs) | |||
|
Merging so we can release today |
Description of changes
Summarize the changes made by this PR.
- When (for some reason) the tracer is not defined, we seem to be returning None, added original function return for completeness
Test plan
How are these changes tested?
Manual testing with Zipkin/Jaeger
Documentation Changes
N/A