Make otel spans for RR clients more spec compliant #26694
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @radcortez |
This comment has been minimized.
This comment has been minimized.
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
2f50b08 to
bcdfc3f
Compare
This comment has been minimized.
This comment has been minimized.
|
The test failures seems related to the change |
|
Yep. This is me juggling the separate changes and forgetting to rebuild and test both modules. |
|
This new output format is uses the route template which is only available after the other PR. |
|
At the very least, using this name format, the the handler order is tested. If the route template becomes unavailable again due to a code change it should show up in the OpenTelemetry tests. |
radcortez
requested changes
Jul 14, 2022
...main/java/io/quarkus/opentelemetry/runtime/tracing/restclient/OpenTelemetryClientFilter.java
Show resolved
Hide resolved
...main/java/io/quarkus/opentelemetry/runtime/tracing/restclient/OpenTelemetryClientFilter.java
Outdated
Show resolved
Hide resolved
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
bcdfc3f to
fe31790
Compare
|
The tests are fixed now that the other PR is merged. |
This comment has been minimized.
This comment has been minimized.
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
fe31790 to
5d2dddb
Compare
radcortez
requested changes
Jul 15, 2022
...main/java/io/quarkus/opentelemetry/runtime/tracing/restclient/OpenTelemetryClientFilter.java
Outdated
Show resolved
Hide resolved
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
5d2dddb to
56c4353
Compare
|
@radcortez As discussed this removes the custom extractor and replaces it with the default |
radcortez
approved these changes
Jul 22, 2022
|
Thank you for the PR @kdubb! |
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
69b90c7 to
1049658
Compare
kdubb
added
triage/waiting-for-ci
The OpenTelementry [Spec](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.10.0/specification/trace/semantic_conventions/http.md#name) is pretty clear about not using the URI as the default span name due to its high cardinality. > Instrumentation MUST NOT default to using URI path as span name… This PR creates span names that always include `HTTP` and then if available the method (e.g. `GET`) or `request`.
kdubb
force-pushed
the
fix/otel_rr_client_span_name
branch
from
1049658 to
3ce1ac2
Compare
quarkus-bot
bot
removed
the
triage/waiting-for-ci
|
@radcortez Kevin would like me to backport this one. I'm not sure if we want to change this behavior in a micro. But maybe it makes sense given the previous behavior was not the one we wanted? |
|
I think it is ok to backport. |
michalvavrik
added a commit
to michalvavrik/quarkus-test-suite
that referenced
this pull request
Jul 27, 2022
Logic behind Rest Client span names has changed in quarkusio/quarkus#26694, this reflect changes and fixes related test failures in daily CI.
8 tasks
fedinskiy
pushed a commit
to quarkus-qe/quarkus-test-suite
that referenced
this pull request
Jul 27, 2022
Logic behind Rest Client span names has changed in quarkusio/quarkus#26694, this reflect changes and fixes related test failures in daily CI.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The OpenTelementry Spec is pretty clear about not using the URI as the default span name due to its high cardinality.
Previously that's exactly what the span name extractor was doing.
Understanding that there are a number of issues and PRs related to reducing the cardinality of the client span names. After working in a live environment with telemetry enabled I've made some changes to make the span names compliant and easier to disambiguate in UIs like Jaeger.
HTTPfollowed by the method, when available.Examples:
Always prepending the
HTTP <method>disambiguates the client and server spans (which might share the same exact route template) in the UI. In the example below you can see how the edge service uses a client to call the data service. The client and server have the exact same route template but with the prefixing it easily distinguishable.This example might seem a little redundant since it's in the hierarchical format but we find it much easier to read. Also, in other flat formats the client and server are quite clear without drilling down.