Meaningful message when span name is not a string #3955
Conversation
| logger.warning( | ||
| "span name must be a string.\n Stack trace: %s", stack_trace | ||
| ) | ||
| name = str(name) |
There was a problem hiding this comment.
Did you consider cases where converting the name to a string might fail?
There was a problem hiding this comment.
I cannot really think of any case where this could fail. Did you have any such cases in mind?
There was a problem hiding this comment.
It's good that we're logging something, but I'm not sure that we want to log a stack trace here. Is logging a stacktrace done elsewhere in this repo?
Regarding testing, I think we should test passing in a string, and test passing in a non-string, making sure both scenarios produce expected results.
There was a problem hiding this comment.
I don't see logging a stacktrace being done anywhere in the repo. Would you like me to change it and if so, how would you like me to change it?
Sure, working on it now.
There was a problem hiding this comment.
Maybe this should just raise a TypeError. Anybody have thoughts?
if not isinstance(name, str):
raise TypeError(f"Expected 'name' to be a string, but got {type(name).__name__}")
There was a problem hiding this comment.
Seems like there's two thing we can do:
- Treat this as a misconfiguration/misuse of the api (even though it's not explictly stated that span name is a string) similar to here and raise a typeerror in the create span api
- Handle this gracefully and log a warning instead -> attempt to convert the value to a str in the api and throw an error if this attempt fails.
I personally am more for (1), this seems more like a misuse of an api in which every we expect name to be a string. Otherwise, we must do type checks every we use span name.
There was a problem hiding this comment.
Raising an error here is against the OTel basic error handling principles i believe. Since ReadableSpan.__init__ is called on Span.end a raised error will escape the API/SDK and fail the user's application.
Converting to str is probably fine but should be handled with care since e.g. a custom __str__ implementation might also raise an error.
Another option might be to just replace the span name with some hardcoded "i'm not a str pls fix me" message so users would see in their backend that something isn't quite right.
There was a problem hiding this comment.
Given the specification quotes here #3955 (comment) and that we have the proper type on the method parameter I'll just leave the code as is. Hopefully in the future users would be able to catch these with a type checker?
There was a problem hiding this comment.
Should we close the pr in this case? @mariojonke wdyt
There was a problem hiding this comment.
Should we close the pr in this case? @mariojonke wdyt
Yes
|
I'll be closing this pr due to this comment, feel free to comment if you want to continue this discussion. |
Description
When the span name is not a string, the error message is confusing. Logs a warning message "span name must be a string" with the stack trace to help identify where the issue is. Converts the span name to a string and continues execution.
Fixes #3918
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Ran the code given in the issue to reproduce the issue.
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/oropentelemetry-sdk/The method interfaces of
test/utilhave changedScripts in
scripts/that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.tomlisort.cfg.flake8When a new
.github/CODEOWNERis addedMajor changes to project information, such as in:
README.mdCONTRIBUTING.mdYes. - Link to PR:
No.
Checklist: