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

Server Instrumentation of io.opentelemetry.netty-4.1 populates tag http.route always with / only #7350

Closed
arik-dig opened this issue Dec 1, 2022 · 25 comments
Labels
bug Something isn't working needs author feedback Waiting for additional feedback from the author repro provided stale

Comments

@arik-dig
Copy link
Contributor

arik-dig commented Dec 1, 2022

Describe the bug
Server instrumentation library io.opentelemetry.netty-4.1 using java agent sets the Span name and populate the tag http.route always as /
while ignoring the declaration of the Controller mapping

Steps to reproduce
using project https://github.com/quarkusio/quarkus-quickstarts/tree/main/spring-web-quickstart

which contains spring controller:

@RestController
@RequestMapping("/greeting")
public class GreetingController {
    @GetMapping("/{name}")
    public Greeting hello(@PathVariable(name = "name") String name) {
    }
}

I ran the project using the agent:

java \
 -javaagent:target/otel/opentelemetry-javaagent.jar \
 -jar target/quarkus-app/quarkus-run.jar

and browsed locally to http://localhost:8080/greeting/mark

What did you expect to see?

  1. Span name should be /greeting/{name}
  2. Value of tag http.route should be /greeting/{name}

What did you see instead?

  1. Span name is /
  2. Value of tag http.route is /

What version are you using?
java agent 1.20.2-alpha

Environment
Compiler: Oracle JDK 17
OS : Windows 11
Runtime : Oracle Corporation Java HotSpot(TM) 64-Bit Server VM 17.0.5+9-LTS-191
OS : Windows 11

Additional context

  • Span tags :
tag value  
http.flavor 1.1  
http.method GET  
http.response_content_length 25  
http.route /  
http.scheme http  
http.status_code 200  
http.target /greeting/mark  
internal.span.format proto  
net.host.name localhost  
net.host.port 8080  
net.sock.family inet6  
net.sock.host.addr 0:0:0:0:0:0:0:1  
net.sock.peer.addr 0:0:0:0:0:0:0:1  
net.sock.peer.port 59992  
net.transport ip_tcp  
otel.library.name io.opentelemetry.netty-4.1  
otel.library.version 1.20.2-alpha  
span.kind server  
thread.id 51  
thread.name vert.x-eventloop-thread-19
  • Resource tags:
tag value  
host.arch amd64  
os.description Windows 11 10.0  
os.type windows  
process.command_line C:\Program Files\Java\jdk-17.0.5\bin\java.exe -javaagent:target/otel/opentelemetry-javaagent.jar  
process.executable.path C:\Program Files\Java\jdk-17.0.5\bin\java.exe  
process.runtime.description Oracle Corporation Java HotSpot(TM) 64-Bit Server VM 17.0.5+9-LTS-191  
process.runtime.name Java(TM) SE Runtime Environment  
process.runtime.version 17.0.5+9-LTS-191  
telemetry.auto.version 1.20.2  
telemetry.sdk.language java  
telemetry.sdk.name opentelemetry  
telemetry.sdk.version 1.20.1
@arik-dig arik-dig added the bug Something isn't working label Dec 1, 2022
@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 1, 2022

attaching the Trace (exported from jaeger, as JSON format):
trace_8447c7c52ee622d325208e1792fc724f.json.txt

@laurit
Copy link
Contributor

laurit commented Dec 2, 2022

Netty is low level framework that does not provide mappings that could be used for route. http.route is usually handled by a higher level framework like quarkus. Perhaps @brunobat knows whether there is an instrumentation for quarkus that could fill http.route, or whether such instrumentation is planned.

@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 4, 2022

Should this be added by Quarkus or by Web MVC which was used by my code?

@brunobat
Copy link

brunobat commented Dec 5, 2022

@laurit @arik-dig Quarkus is currently not always setting the http.route property.
Yes, this is handled at a higher level... and should be reported as a Quarkus bug.

@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 5, 2022

@brunobat we're trying to run Quarkus and OTEL but currently the Span name and http.route attribute are contextless.
Whats the best way to promote and prioritize a fix for it?
where should the bug be opened?
Thanks

@brunobat
Copy link

brunobat commented Dec 5, 2022

Actually, there is one already:
quarkusio/quarkus#29297
Please add a comment with tour case.

@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 5, 2022

@brunobat , @laurit - just to emphasize I'm using the java agent (auto instrumentation)
and NOT using the Quarkus instrumentation (Quarkus extension for OTEL)
I don't think its related to Quarkus issue 29297, is it?

@brunobat
Copy link

brunobat commented Dec 5, 2022

@arik-dig not sure you will ever be able to get the http.route with Netty alone.
The HTTP routing is performed at the vert.x level, not Netty.
The linked issue is a link to a Quarkus bug because this is not currently working in the built in Quarkus instrumentation itself.
The Otel Agent is not recommended with Quarkus due to complex context switching issues affecting the agent.

@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 5, 2022

@arik-dig The Otel Agent is not recommended with Quarkus due to complex context switching issues affecting the agent.

@brunobat its quite a concern, is that documented somewhere?
Then what is the right approach , should we use the Quarkus-Otel extension, instead of the Agent?

@brunobat
Copy link

brunobat commented Dec 5, 2022

@arik-dig Quarkus allows you to mix reactive and imperative programming paradigms. Preserving the OTel context when switching between those 2 worlds is very complex and not properly handled by the agent.
In Short... I you only use imperative style, the Agent should be ok, if not, only the Quarkus extension is an option.
Also, with the extension you get some nice perks like span ids in the logs. Give it a try.

@arik-dig
Copy link
Contributor Author

arik-dig commented Dec 5, 2022

@brunobat thanks for the info

@trask
Copy link
Member

trask commented Dec 5, 2022

@arik-dig The Otel Agent is not recommended with Quarkus due to complex context switching issues affecting the agent.

@brunobat its quite a concern, is that documented somewhere?

@brunobat can you open an issue and post a repro of a context switching issue so we can investigate from the agent side? thx!

@arik-dig
Copy link
Contributor Author

@trask , @mateuszrzeszutek - this issue is not directly related to Quarkus.
it seems like netty instrumentation and the higher level instrumentations(spring web, vertx web) do not modify the span name + 'http.route' attribute.
can you please address it?

@trask
Copy link
Member

trask commented Dec 12, 2022

@arik-dig can you post a small repro of the issue? generally http.route will be captured by the webflux or vertx instrumentation if you are using one of those libraries, but it's certainly possible that there's a case where it's missing

@arik-dig
Copy link
Contributor Author

@trask I've provided a repro - using Quarkus app (Spring WEB) with OTEL agent (see in the description).

@trask
Copy link
Member

trask commented Apr 2, 2023

thx @arik-dig, I was able to repro, it looks like quarkus supports spring annotations, but has its own routing implementation (instead of using spring mvc routing, which is what we currently instrument).

@trask
Copy link
Member

trask commented Apr 2, 2023

@arik-dig The Otel Agent is not recommended with Quarkus due to complex context switching issues affecting the agent.

@brunobat its quite a concern, is that documented somewhere?

I checked and the repro is not losing the context, as I was able to update the netty span name manually inside of the controller method, e.g. Span.current().updateName(...)

@brunobat can you open an issue and post a repro of a context switching issue so we can investigate from the agent side? thx!

@brunobat we would really appreciate if you can provide a repro of the issue(s) you have seen so we can look into them, thx!

@trask
Copy link
Member

trask commented Aug 24, 2023

@arik-dig wondering if this was resolved by #8487?

@trask trask added needs repro needs author feedback Waiting for additional feedback from the author and removed needs repro labels Aug 24, 2023
@arik-dig
Copy link
Contributor Author

@trask I'm not sure.
Actually it is not recommended in Quarkus to use the OTEL agent.
they use OTEL SDK without the agent.

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Aug 29, 2023
@trask
Copy link
Member

trask commented Aug 30, 2023

@trask I'm not sure. Actually it is not recommended in Quarkus to use the OTEL agent. they use OTEL SDK without the agent.

is this the issue you're facing? quarkusio/quarkus#29297

@trask trask added the needs author feedback Waiting for additional feedback from the author label Aug 30, 2023
@arik-dig
Copy link
Contributor Author

@trask I think that quarkus people can answer it better.

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Aug 31, 2023
@brunobat
Copy link

brunobat commented Aug 31, 2023

We need to re-visit the fundamentals of this issue and what we want to achieve.

  1. I wouldn't expect a http.route to be set at the Netty level.
  2. Quarkus has changed quite a lot since the creation of this issue and now http.route seems to be properly set across the board.
  3. I see in one of the comments and the actual issue description that this might actually be related with the agent and the OTel bundled Netty instrumentation.
  4. Quarkus doesn't use the agent and should not be instrumented with the Agent. A root flaw in the issue. If this is about the Agent in Quarkus, the issue should be closed.

@arik-dig can you please re-write the issue so the scope and purpose are clarified?

@trask
Copy link
Member

trask commented Aug 31, 2023

Quarkus doesn't use the agent and should not be instrumented with the Agent. A root flaw in the issue. If this is about the Agent in Quarkus, the issue should be closed.

@brunobat this is not true from the agent side at least, while we love that Quarkus has native instrumentation and we highly recommend using it, we do support Quarkus users who need to use the agent for some reason, e.g. they are instrumenting an existing app, or they need auto-instrumentation of additional libraries.

But I do agree that we need clarification here @arik-dig about whether you are using the agent or not.

And both the agent and Quarkus have made improvements in this area since this issue was opened:

So it would be helpful if you can check if your issue still exists using the latest version of whichever path you are taking.

@trask trask added the needs author feedback Waiting for additional feedback from the author label Aug 31, 2023
@brunobat
Copy link

That's fine Trask.
The problem I see relates to the multiple combinations of the quarkus-opentelemetry extension and the Agent...

  • If you use only the agent, OTel instrumentation libs will be used
  • If you use the extension, it will use whatever instrumentation is built in in Quarkus
  • If you use Agent+extension, It will get messy. Never tested it and most likely the results will be a bit surprising and not recommended at all.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

This has been automatically marked as stale because it has been marked as needing author feedback and has not had any activity for 7 days. It will be closed automatically if there is no response from the author within 7 additional days from this comment.

@github-actions github-actions bot added the stale label Sep 7, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs author feedback Waiting for additional feedback from the author repro provided stale
Projects
None yet
Development

No branches or pull requests

5 participants