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.

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

Define the fallback tracer name for invalid values. #1534

Merged

Conversation

carlosalberto
Copy link
Contributor

@carlosalberto carlosalberto commented Mar 11, 2021

Fixes #1472

Keeps the original invalid value (to be handled by the backends instead), while asking implementations to log this as an error.

@carlosalberto carlosalberto requested a review from a team as a code owner March 11, 2021 14:43
@carlosalberto carlosalberto requested a review from a team March 11, 2021 14:43
@carlosalberto carlosalberto requested a review from a team as a code owner March 11, 2021 14:43
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry. Thanks!

specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Personally I would prefer to use "" as a fallback and treat that as invalid value. That way we are consistent with our protocol as well.

@yurishkuro
Copy link
Member

I think it's worth comparing this with a missing service name, which is quite bad and we already have some fallback value for it. A missing/blank instrumentation library feels very much meh, i.e. a blank string would do just fine as a fallback with very little downside.

carlosalberto and others added 2 commits March 11, 2021 18:29
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor Author

I think it's worth comparing this with a missing service name

Indeed, I had considered this as well, and was wondering whether we should try to go with "unknown_name" or a similar one, to be in line with that a missing service name ends up doing ;)

@Oberon00
Copy link
Member

The now says that a missing service name is not an error (just an unknown service name), while a missing instrumentation name is an API usage error. Note that a missing service name naturally appears by doing nothing in particular while you have to decide to pass an empty string or null to GetTracer to get an invalid instrumentation name.

null or throwing an exception.
default Tracer implementation MUST be returned as a fallback rather than returning
null or throwing an exception, and `name` SHOULD be the set to the
"`<INVALID INSTRUMENTATION NAME PROVIDED>`" literal, in order to signal that the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"`<INVALID INSTRUMENTATION NAME PROVIDED>`" literal, in order to signal that the
string `<INVALID INSTRUMENTATION NAME PROVIDED>`, in order to signal that the

@arminru
Copy link
Member

arminru commented Mar 16, 2021

@carlosalberto Please add a changelog entry that this fallback is now specified for SDK implementations.

@bogdandrutu
Copy link
Member

@Oberon00 @carlosalberto can you please document why empty string is not good? When it comes to receiving empty string from a source what does that mean?

I would vote for empty string since it means less unnecessary bytes on the wire and this case has to be treated by the receiver anyway.

@carlosalberto
Copy link
Contributor Author

From the discussion today: we want to allow the user passing null or empty strings, as this is something that can/should be handled by the backend (as backends must very likely already handle the null value case), while logging (even if only once) about the invalid name value.

@carlosalberto
Copy link
Contributor Author

@open-telemetry/specs-approvers Please re-review ;)

specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member

@carlosalberto pls resolve couple suggestions from @yurishkuro before merging. Thank you!

carlosalberto and others added 2 commits March 17, 2021 14:54
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define the default value for null/empty instrumentation-name
8 participants