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

Missing http.route attribute on OpenTelemetry server spans #27827

Closed
knutwannheden opened this issue Sep 9, 2022 · 7 comments
Closed

Missing http.route attribute on OpenTelemetry server spans #27827

knutwannheden opened this issue Sep 9, 2022 · 7 comments
Labels
area/tracing kind/bug Something isn't working

Comments

@knutwannheden
Copy link
Contributor

Describe the bug

As commented here #26776 (comment) the PR #26898 causes a regression in Quarkus 2.12.1: The HTTP server spans no longer include the http.route attribute and consequently the span names are now reduced to HTTP <METHOD> rather than including the HTTP route.

Expected behavior

AFAICT the issue #26776 requested that HTTP requests which result in a 400 or 501 should use a low-cardinality span name rather than the full (not successfully routed) request URI. I think that makes sense, but for successfully routed requests, I think the spans should include the standard http.route attribute and use that as part of the span name.

Actual behavior

All HTTP server spans have names like HTTP GET or HTTP POST which don't include the HTTP route. Also, the spans don't contain the http.route attribute.

How to Reproduce?

The simplest way to reproduce this is probably to write a test using InMemorySpanExporter.

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.12.1.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@knutwannheden knutwannheden added the kind/bug Something isn't working label Sep 9, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 9, 2022

/cc @brunobat, @radcortez

@radcortez
Copy link
Member

Are your REST endpoints defined in interfaces?

It seems that our changes uncovered a side effect. Before, we were always generating the route and then updating it with the one coming from the context. Now, we just update it with the one coming from the context. The issue is that the route path is set via a CDI interceptor that is not executed when the REST annotations are placed in an interface.

@knutwannheden
Copy link
Contributor Author

Are your REST endpoints defined in interfaces?

No, the endpoints are defined in regular classes.

It seems that our changes uncovered a side effect. Before, we were always generating the route and then updating it with the one coming from the context. Now, we just update it with the one coming from the context. The issue is that the route path is set via a CDI interceptor that is not executed when the REST annotations are placed in an interface.

I will try to figure out what is going on here.

@radcortez
Copy link
Member

Weird. There is definitely a problem when REST endpoints are defined in interfaces, but regular endpoints should update the route name correctly. I've tried with a few sample endpoints. When we did the change, it didn't break the tests either, so maybe there are some other conditions that we are not picking up.

If you can get me a reproducer, it would be great.

@knutwannheden
Copy link
Contributor Author

It appears to be working correctly after all. I had some code which expected the HTTP route to be available already before the request completes (and the attribute gets updated). This was in the context on an interceptor on the JAAS annotations (@RolesAllowed etc.), which I use to perform "security logging".

I can however live with the fact that the HTTP route is not yet available until the operation completes.

@knutwannheden
Copy link
Contributor Author

@radcortez Before stumbling over this I was however thinking about reporting a feature request to also get an http.route attribute for client spans, when these originate from an MP REST Client, where a route could be derived from the annotations. If you think this makes sense, I can create an issue.

@radcortez
Copy link
Member

We had that before, but we decided to remove it in #26694. Please, check the following conversation: #26694 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tracing kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants