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

Fix Operation name logging for Netty based applications #2317

Merged
merged 8 commits into from Feb 19, 2021
Merged

Fix Operation name logging for Netty based applications #2317

merged 8 commits into from Feb 19, 2021

Conversation

neinoi
Copy link
Contributor

@neinoi neinoi commented Feb 17, 2021

Fix Operation name logging for Netty based applications

(@trask Report of PR #1494 on ApplicationInsights-Java repo)

@laurit
Copy link
Contributor

laurit commented Feb 17, 2021

See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md

Instrumentation MUST NOT default to using URI path as span name

@neinoi
Copy link
Contributor Author

neinoi commented Feb 17, 2021

@laurit if I follow your recommandation, all my operations will be « HTTP GET » … so I can let « netty.request » the result is the same.

These are more usefull : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span

Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

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

As @laurit correctly noticed, using full URL (even without query string) violates Otel semantic convention and general requirement for low cardinality of the span names.

@neinoi
Copy link
Contributor Author

neinoi commented Feb 17, 2021

Here without my modifications :

image
I agree, for low cardinality it’s perfect, all calls have the same name.

Here after my modifications (first version, before my last modifications) :
image
It’s not perfect, but I think a bit better.

@trask
Copy link
Member

trask commented Feb 18, 2021

Hi @neinoi! As appealing as it is to use the path as the span name, it doesn't work well for applications that have an unbounded number of (parameterized) paths.

Since the one-size-fits-all span name is not that great (and we all agree that HTTP GET is not that great 😄), there are ways for applications to customize the span name.

Since you are using Application Insights, if you open an issue in that repo, I can explain the different options for customizing the span name (which are a bit specific to Application Insights, since we don't use the OTel Collector, and our OTel API interoperability is under a preview flag).

If you'd like to continue with this PR, changing the span name to HTTP {method} would be great, we missed netty.request previously when updating to the OpenTelemetry semantic conventions, and it would be a good update this.

@iNikem iNikem self-requested a review February 18, 2021 13:45
@neinoi
Copy link
Contributor Author

neinoi commented Feb 18, 2021

Hi @trask ,
I’ve modified the code to have HTTP {method} for the span name.
For the options, is this the good place to go : java-standalone-telemetry-processors ?
(I will transmit these informations to my successor in my current project)

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

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.

None yet

5 participants