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

Wrong span names for server HTTP spans #391

Closed
iNikem opened this issue May 7, 2020 · 5 comments · Fixed by #428
Closed

Wrong span names for server HTTP spans #391

iNikem opened this issue May 7, 2020 · 5 comments · Fixed by #428
Assignees

Comments

@iNikem
Copy link
Contributor

iNikem commented May 7, 2020

Can be considered as part of #233.

Several auto-instrumentations which create "server http" spans currently use simply "HTTP $method" as span name. This seem to violate the spec. E.g. Servlet2Advice and Servlet3Advice

@iNikem
Copy link
Contributor Author

iNikem commented May 8, 2020

In contrast with situations, when there is some middleware that would produce nice span names (such as Spring MVC'c MethodHandlers), the instrumentations of plain servlets or Netty pipelines have only requested URL as a potential source of span names. Should we try to somehow normalise URL to remove potential high cardinality parts of it? E.g. SignalFx uses the following regex for this:

  // Matches any path segments with numbers in them. (exception for versioning: "/v1/")
  public static final Pattern PATH_MIXED_ALPHANUMERICS =
      Pattern.compile("(?<=/)(?![vV]\\d{1,2}/)(?:[^\\/\\d\\?]*[\\d]+[^\\/\\?]*)");

In case of servlets, we can potentially use class name + method name as well.

@trask
Copy link
Member

trask commented May 11, 2020

I don't think(?) "HTTP $method" violates the spec for server spans:

If the route cannot be determined, the name attribute MUST be set as defined in the general semantic conventions for HTTP.

And in that general section:

HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}"

I agree that "HTTP $method" is not a terribly helpful grouping for users though ☹️.

Maybe this is a discussion for oteps/specification?

@iNikem
Copy link
Contributor Author

iNikem commented May 11, 2020

I think that specification is good enough on this topic. It is our current implementation that is lacking. We have to make more efforts in producing useful span names. Maybe giving a configuration option to choose if application owner wants to use url-based span names?

@iNikem
Copy link
Contributor Author

iNikem commented May 11, 2020

Ok, one specific proposal: to change span names for servlet-based http spans to ServletClassName.methodName. Yes/No?

The same can be done for GrizzlyHttpHandlerInstrumentation as well. Yes/No?

@trask
Copy link
Member

trask commented May 11, 2020

There's some related discussion on preferring route over class.method: open-telemetry/opentelemetry-specification#548

But that discussion doesn't address what to do if there's no route, in which case class.method seems like a better alternative to "HTTP $method".

So "yes" from me to your Yes/No questions above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants