Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Meaningful message when span name is not a string #3955

Closed
wants to merge 8 commits into from
Closed

Meaningful message when span name is not a string #3955

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e84e823
span non string name issue solve
soumyadeepm04 Jun 5, 2024
401ff61
fixed linting issues
soumyadeepm04 Jun 5, 2024
b2737ce
fixed linting issues
soumyadeepm04 Jun 5, 2024
8db1003
added test cases
soumyadeepm04 Jun 6, 2024
75bffd7
raise typeerror when span has non string name
soumyadeepm04 Jun 12, 2024
6788c15
Merge branch 'main' into main
soumyadeepm04 Jul 12, 2024
05061af
Merge branch 'main' into main
emdneto Jul 19, 2024
a7ed812
Merge branch 'main' into main
emdneto Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,12 @@ def __init__(
end_time: Optional[int] = None,
instrumentation_scope: Optional[InstrumentationScope] = None,
) -> None:
if not isinstance(name, str):
stack_trace = "".join(traceback.format_stack())
logger.warning(
"span name must be a string.\n Stack trace: %s", stack_trace
)
name = str(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider cases where converting the name to a string might fail?

Copy link
Contributor Author

@soumyadeepm04 soumyadeepm04 Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot really think of any case where this could fail. Did you have any such cases in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__}")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's two thing we can do:

  1. 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
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xrmx

Should we close the pr in this case? @mariojonke wdyt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we close the pr in this case? @mariojonke wdyt

Yes

self._name = name
self._context = context
self._kind = kind
Expand Down
Loading