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

OpenTelemetry: High span name cardinality for invalid HTTP request spans #26776

Closed
calohmn opened this issue Jul 18, 2022 · 3 comments · Fixed by #26898
Closed

OpenTelemetry: High span name cardinality for invalid HTTP request spans #26776

calohmn opened this issue Jul 18, 2022 · 3 comments · Fixed by #26898
Assignees
Labels
area/tracing kind/bug Something isn't working
Milestone

Comments

@calohmn
Copy link

calohmn commented Jul 18, 2022

Describe the bug

This is about the remaining issue from #26216:
When using the quarkus-opentelemetry extension and observing Vert.x HTTP server request spans, the span names for invalid HTTP requests (resulting in status 400 or also 501 responses) contain the full request URI path. The OTEL spec declares such high cardinality span names as unsuitable.

In practice, this means the span selection combobox in the Jaeger UI gets fill with all kinds of weird URI paths of bot requests trying to exploit some vulnerability.

/someWeirdPathTryingToExploitSomeVulnerability?someParamX

Some of these span names still can get renamed by using a "match-all" Vert.x route with a failure handler (invoking ctx.request().routed(name) there),

But there are kinds of invalid HTTP requests, for which no route failure handler will get invoked. This happens for example with HTTP requests with an invalid Content-Length header value.

Expected behavior

I would expect some fixed span name (e.g. the HTTP request method name) to be used as span name for such invalid HTTP requests - if no request route name got set/applied (i.e. no named route got matched and ctx.request().routed(name) wasn't invoked).

Actual behavior

High cardinality span names are used.

How to Reproduce?

Make invalid HTTP requests to a quarkus-opentelemetry application having a Vert.x HTTP server endpoint and observe created span names.
Examples:
printf 'GET invalid__no_leading_slash HTTP/1.0\r\n\r\n' | nc -q 3 localhost 80
printf 'GET /something%%%%3 HTTP/1.0\r\n\r\n' | nc -q 3 localhost 80

printf 'GET /something HTTP/1.1\r\nContent-Length: 18446744073709551615\r\n\r\n' | nc -q 3 localhost 80
=> no Vert.x route getting invoked

printf 'GET /something HTTP/5.0\r\n\r\n' | nc -q 3 localhost 80
=> this actually returns status 501; no Vert.x route getting invoked

Output of uname -a or ver

No response

Output of java -version

openjdk 17.0.3 2022-04-19

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.10.2

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

No response

Additional information

No response

@calohmn calohmn added the kind/bug Something isn't working label Jul 18, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 18, 2022

/cc @radcortez

@knutwannheden
Copy link
Contributor

@radcortez Looking at the implementation it seems like now the span name will always just be HTTP <METHOD> and never actually include the route. Further, since ServerAttributesExtractor#route(HttpRequest) always returns null, there will also not be any http.route attribute set on the span. To me this seems wrong. The way I interpreted this issue, the request was to only omit the route for erroneous requests, in which case there is no real route. Please let me know if I should file a new issue for this regression.

@radcortez
Copy link
Member

No, this is fine. The route is set by

HttpRouteHolder.updateHttpRoute(spanOperation.getSpanContext(), FILTER, RouteGetter.ROUTE_GETTER,
.

The issue you see is something else. I'll reply in #27827.

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

Successfully merging a pull request may close this issue.

4 participants