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

Akka http server span names are always akka.request #3478

Closed
mattypb opened this issue Jul 2, 2021 · 14 comments
Closed

Akka http server span names are always akka.request #3478

mattypb opened this issue Jul 2, 2021 · 14 comments
Assignees
Labels

Comments

@mattypb
Copy link

mattypb commented Jul 2, 2021

Describe the bug
I've added the Open Telemetry java instrumentation to a Scala Akka service. Traces are created when the service's api's are hit, but no matter which api is hit they all have a span name of akka.request.

Looking at the Akka http server instrumentation, this is appears to be the current expected behaviour - AkkaHttpServerInstrumentationTest. However some other java instrumentations use the api path as the span name, for example Tomcat 7.0 - TomcatHandlerTest.

Why does the Akka server instrumentation have akka.request hard coded for the span name instead of using the api path? Will this change in the future?

What did you expect to see?
Hitting an api on my service, e.g. GET /v1/{user}, would generate a trace with span name /v1/{user}

What did you see instead?
Span name of akka.request

What version are you using?
1.3.0

Environment
Scala: 2.13.5
Akka-Http: 10.2.4
Compiler: AdoptOpenJDK 11.0.10
OS: Mac 10.15.7
Runtime (if different from JDK above): Oracle 11.0.11+9-LTS-194
OS (if different from OS compiled on): RHEL 7.1

@mattypb mattypb added the bug Something isn't working label Jul 2, 2021
@nibhuran
Copy link

@mattypb , @trask Can I take a crack at this issue.

@trask
Copy link
Member

trask commented Sep 30, 2021

Can I take a crack at this issue.

sure!

@mcmho
Copy link
Contributor

mcmho commented Jan 7, 2022

@trask, I am looking into this a bit. Does it make sense to add a SpanNameExtractor for this case and pass it to the Instrumenter?

thoughts? @mateuszrzeszutek

@trask
Copy link
Member

trask commented Jan 7, 2022

@trask, I am looking into this a bit. Does it make sense to add a SpanNameExtractor for this case and pass it to the Instrumenter?

yes, that makes sense 👍

@mcmho
Copy link
Contributor

mcmho commented Jan 12, 2022

@trask, @mateuszrzeszutek, I see that there are many span name extractors having null implementation for route().

  @Override
  @Nullable
  protected String route(Request request) {
    return null;
  }

I realized that it is hard to get the right span name with low cardinality with just the Request object.
Do you have any advice on that?

In our company's product, we provide configuration where customers can specify their own custom rule to name a span.
or we provide a default... say for example, the servlet or http case, we use the first two segments of the URI.
so an uri like this one: http://localhost:11001/foo/bar/baz?id=66
the span name would be /foo/bar

I am wondering if that would be sufficient. On the other hand, I understand that
the second segment can potentially be an id. in that case, we will end up with a span name that has high cardinality.

Does it make sense to parse the second segment skip it if it is a purely digit?

@trask
Copy link
Member

trask commented Jan 12, 2022

hey @mcmho!

I realized that it is hard to get the right span name with low cardinality with just the Request object.
Do you have any advice on that?

if the mvc framework that we are instrumenting has a routing concept, then we use that (e.g. spring's @RequestMapping).

if the mvc framework has no routing concept then we don't capture any route, and the span name is the mostly unhelpful (but guaranteed low-cardinality) HTTP {method}.

It looks like akka http does have a route concept (https://doc.akka.io/docs/akka-http/current/routing-dsl/routes.html). if we can capture the route from akka that would be ideal

In our company's product, we provide configuration where customers can specify their own custom rule to name a span.
or we provide a default... say for example, the servlet or http case, we use the first two segments of the URI.
so an uri like this one: http://localhost:11001/foo/bar/baz?id=66
the span name would be /foo/bar

ya, this is a nice option. I suspect we may introduce something like this in the future (or possibly this could be supported as post-processing in the collector).

mcmho added a commit to mcmho/opentelemetry-java-instrumentation that referenced this issue Jan 17, 2022
 - removed hardcoded span name "akka.request"
 - implemented route() in AkkaHttpServerAttributesExtractor to provide a better span name
 - retrofitted AkkaHttpServerInstrumentationTest.groovy
@mcmho
Copy link
Contributor

mcmho commented Jan 17, 2022

Thanks @trask! I did look into the Route concept. As far as I understand, Route is the static configuration logic rather the runtime request route. It seems to be used for constructing a mapping to route request to the handling logic. i.e: https://developer.lightbend.com/guides/akka-http-quickstart-java/http-server.html
I can't find a way to get Route from HttpRequest.

My approach is to inspect the path in the request and come up with a span name with low cardinality.

@trask
Copy link
Member

trask commented Jan 18, 2022

My approach is to inspect the path in the request and come up with a span name with low cardinality.

limiting the path to two segments doesn't guarantee low-cardinality, which is a requirement for span name

@mcmho
Copy link
Contributor

mcmho commented Jan 18, 2022

@trask , I understand that the second segment can potentially be an id. in that case, we will end up with a span name that has high cardinality.

Does it make sense to parse the second segment skip it if it is a purely digit?

@trask
Copy link
Member

trask commented Jan 18, 2022

I think that if we can't capture the real akka route that we should probably stick to capturing just HTTP {method} in the akka instrumentation itself

@mcmho
Copy link
Contributor

mcmho commented Jan 18, 2022

Okay, I will do HTTP GET, HTTP POST, ... etc for now.

Wouldn't it be great if we can allow some sort of customization rules in the collector to post process span names,
such that users can name their span say /foo/10, /foo/11, as /foo
or /bar/111, /bar/123 as /bar.

mcmho added a commit to mcmho/opentelemetry-java-instrumentation that referenced this issue Jan 19, 2022
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy
mcmho added a commit to mcmho/opentelemetry-java-instrumentation that referenced this issue Jan 19, 2022
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy
mcmho added a commit to mcmho/opentelemetry-java-instrumentation that referenced this issue Jan 19, 2022
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy and related tests
@mcmho
Copy link
Contributor

mcmho commented Jan 19, 2022

@trask, what about doing something like this:
/foo/10, /foo/11, become /foo/{id}
/bar/111, /bar/123 become /bar/{id}

Or do you just want to stick with HTTP {method}?

@trask
Copy link
Member

trask commented Jan 19, 2022

@trask, what about doing something like this: /foo/10, /foo/11, become /foo/{id} /bar/111, /bar/123 become /bar/{id}

this is hard to do reliably, as not all parameterized path elements have to be numeric (e.g. guid, username, ...)

i think this would be a useful feature to build out separately as a span processor that can apply to all spans, and users can opt into it, and can customize it to their needs. we probably don't have a rich enough configuration model to support this in the agent yet, but in the meantime this could be added in contrib and configured programmatically similar to RuleBasedRoutingSampler

@mcmho
Copy link
Contributor

mcmho commented Jan 19, 2022

Okay, sure @trask. We'll stick with HTTP {method} for this ticket.

And let me take a look at RuleBaseRoutingSampler and create a separate ticket for it.

trask pushed a commit that referenced this issue Jan 20, 2022
* Akka http server span names are always akka.request #3478
 - removed hardcoded span name "akka.request"
 - implemented route() in AkkaHttpServerAttributesExtractor to provide a better span name
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request #3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request #3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request #3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy and related tests
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this issue May 23, 2022
… (open-telemetry#5150)

* Akka http server span names are always akka.request open-telemetry#3478
 - removed hardcoded span name "akka.request"
 - implemented route() in AkkaHttpServerAttributesExtractor to provide a better span name
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request open-telemetry#3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request open-telemetry#3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy

* Akka http server span names are always akka.request open-telemetry#3478
 - removed hardcoded span name "akka.request" and changed to "HTTP {METHOD}"
 - retrofitted AkkaHttpServerInstrumentationTest.groovy and related tests
@trask trask added stale and removed bug Something isn't working labels Aug 26, 2023
@trask trask closed this as completed Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants