open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.92k stars 839 forks source link

HTTP error raised out of controller results in bad transaction.name mapping - quarkus rest reactive client #8873

Closed bcoquell closed 1 year ago

bcoquell commented 1 year ago

Describe the bug When an http 400 error for example is generated outside of the controller of a Quarkus application (client rest reactive in my case) the transaction name is / instead of the /api/name (/v1/profileData). The mapping error comes for example when the error is raised in a method with this kind of priority : @Priority(Priorities.AUTHORIZATION) public class CustomFilter implements ContainerRequestFilter

Steps to reproduce As I made for bug #8319 you will find a sample app here : https://github.com/bcoquell/quarkus-sampleapp-otel-400-badRequest

sample app : https://github.com/bcoquell/quarkus-sampleapp-otel-400-badRequest/blob/main/quarkus-app.zip

Details of observation : https://github.com/bcoquell/quarkus-sampleapp-otel-400-badRequest/blob/main/400-details-and-test-results.pdf

Details to reproduce : https://github.com/bcoquell/quarkus-sampleapp-otel-400-badRequest/blob/main/howto-launch.txt

What did you expect to see? A good mappinf of transaction.name in both cases (400 generated inside/outside the controller)

What did you see instead? / instead of /v1/profileData in the transaction.name

What version are you using? agent opentelemetry 1.27.0

Environment Java / Quarkus 2.7.5 final / Openshift / Elastic APM Server as collector Compiler: (temurin 17.0.6) OS: (Openshift cluster - image built with eclipse-temurin:17)

Additional context Thanks for your help !

bcoquell commented 1 year ago

Hi, I just made a test with 1.28 version of agent, but I still have the problem describe in the ticket when I use my sample app + agent.

image

image

Thanks for your help @laurit @mateuszrzeszutek and all ;)

BR,

Baptiste

laurit commented 1 year ago

@bcoquell could you try the latest snapshot from https://oss.sonatype.org/content/repositories/snapshots/io/opentelemetry/javaagent/opentelemetry-javaagent/1.29.0-SNAPSHOT/ and let us know whether it works now

bcoquell commented 1 year ago

Hi,

OK, I’ll perform the test asap and I’ll come back to you.

Thanks

BR,

-- Baptiste

De : Lauri Tulmin @.> Envoyé : jeudi 20 juillet 2023 19:24 À : open-telemetry/opentelemetry-java-instrumentation @.> Cc : COQUELLE, Baptiste @.>; Mention @.> Objet : Re: [open-telemetry/opentelemetry-java-instrumentation] HTTP error raised out of controller results in bad transaction.name mapping - quarkus rest reactive client (Issue #8873)

This mail has been sent from an external source. Do not reply to it, or open any links/attachments unless you are sure of the sender's identity.

@bcoquellhttps://github.com/bcoquell could you try the latest snapshot from https://oss.sonatype.org/content/repositories/snapshots/io/opentelemetry/javaagent/opentelemetry-javaagent/1.29.0-SNAPSHOT/ and let us know whether it works now

— Reply to this email directly, view it on GitHubhttps://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/8873#issuecomment-1644308369, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKLECXXHM6IOLQFL3QYVVQLXRFSS3ANCNFSM6AAAAAAZ7FWXW4. You are receiving this because you were mentioned.Message ID: @.**@.>> This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.

bcoquell commented 1 year ago

Hi,

I just perform the test with the snapshot opentelemetry-javaagent-1.29.0-20230720.171324-35.jar please find my feedbacks below :

On the first hand, it’s OK for the correction, when 404 is raised out of Controller, especially in our CustomFilter, we now have a correct mapping for transaction.name :

@.***

            And test also still OK for 404 inside the controller, mapping is fine :

@.***

           On the other hand, when the API is not implemented on the application, the transaction.name is always set to / even if the url.path is good as you can see below in trace sample details :

Example of HTTP GET /v1/notImplementedAPI :

@.***

@.***

@.***

            Other example with a healthcheck raised by Openshift, consisting in request HTP GET /status - not implemented on the sample application, it’s tagged as / :

@.***

@.***

            It would be nice to have correct mapping in such cases, for example to identify easily bad client requests for example.

            Is-it possible to check this in the context of the ticket please ? or would you prefer I raise an other one.

            Thanks by advance

BR,

-- Baptiste

De : COQUELLE, Baptiste Envoyé : vendredi 21 juillet 2023 09:42 À : 'open-telemetry/opentelemetry-java-instrumentation' @.>; open-telemetry/opentelemetry-java-instrumentation @.> Cc : Mention @.***> Objet : RE: [open-telemetry/opentelemetry-java-instrumentation] HTTP error raised out of controller results in bad transaction.name mapping - quarkus rest reactive client (Issue #8873)

Hi,

OK, I’ll perform the test asap and I’ll come back to you.

Thanks

BR,

-- Baptiste

De : Lauri Tulmin @.**@.>> Envoyé : jeudi 20 juillet 2023 19:24 À : open-telemetry/opentelemetry-java-instrumentation @.**@.>> Cc : COQUELLE, Baptiste @.**@.>>; Mention @.**@.>> Objet : Re: [open-telemetry/opentelemetry-java-instrumentation] HTTP error raised out of controller results in bad transaction.name mapping - quarkus rest reactive client (Issue #8873)

This mail has been sent from an external source. Do not reply to it, or open any links/attachments unless you are sure of the sender's identity.

@bcoquellhttps://github.com/bcoquell could you try the latest snapshot from https://oss.sonatype.org/content/repositories/snapshots/io/opentelemetry/javaagent/opentelemetry-javaagent/1.29.0-SNAPSHOT/ and let us know whether it works now

— Reply to this email directly, view it on GitHubhttps://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/8873#issuecomment-1644308369, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKLECXXHM6IOLQFL3QYVVQLXRFSS3ANCNFSM6AAAAAAZ7FWXW4. You are receiving this because you were mentioned.Message ID: @.**@.>> This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.

trask commented 1 year ago

It would be nice to have correct mapping in such cases, for example to identify easily bad client requests for example.

I think this would violate the low-cardinality requirement of span names?

Is-it possible to check this in the context of the ticket please ? or would you prefer I raise an other one.

yes, please open a new issue if you'd like to discuss since this is a different (underlying) issue

laurit commented 1 year ago

@bcoquell not implemented endpoints having a route of / is the intended behavior because the specification https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#name requires route to have low cardinality.

bcoquell commented 1 year ago

Hi,

Fine, thank you for the precision and the documentation reference.

BR,

-- Baptiste

De : Lauri Tulmin @.> Envoyé : samedi 22 juillet 2023 08:34 À : open-telemetry/opentelemetry-java-instrumentation @.> Cc : COQUELLE, Baptiste @.>; Mention @.> Objet : Re: [open-telemetry/opentelemetry-java-instrumentation] HTTP error raised out of controller results in bad transaction.name mapping - quarkus rest reactive client (Issue #8873)

This mail has been sent from an external source. Do not reply to it, or open any links/attachments unless you are sure of the sender's identity.

@bcoquellhttps://github.com/bcoquell not implemented endpoints having a route of / is the intended behavior because the specification https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#name requires route to have low cardinality.

— Reply to this email directly, view it on GitHubhttps://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/8873#issuecomment-1646506809, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKLECXVK7X7OIGCIFFL4CPLXRNX5ZANCNFSM6AAAAAAZ7FWXW4. You are receiving this because you were mentioned.Message ID: @.**@.>> This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.