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

TraceFilter creates span names from URIs #500

Closed
s-spindler opened this issue Jan 19, 2017 · 14 comments
Closed

TraceFilter creates span names from URIs #500

s-spindler opened this issue Jan 19, 2017 · 14 comments

Comments

@s-spindler
Copy link

Spans created by TraceFilter have their span names generated from the respective request's full URI including path variables. This results in requests to the same endpoint to have different span names.
For example:
http://localhost:8080/users/abc and http://localhost:8080/users/def would result in http:/users/abc and http:/users/def.
As a result the different spans can not be grouped. Also, zipkin server has problems handling big numbers of span names - at least with a cassandra backend.

It seems more reasonable to put a wildcard in the place of the path variable. In the example above the spans could both be named http:/users/* or something similar.

This would also be more in line with the recommendation in the Spring Cloud Sleuth documentation regarding span naming:

The name should be low cardinality (e.g. not include identifiers).

@marcingrzejszczak
Copy link
Contributor

cc @adriancole

@codefromthecrypt
Copy link
Contributor

FWIW the more important part is usability vs whether a data store can handle a large amount of names. The span name is a grouping label for users, so thousands or millions of choices would neither help in UX, nor help group..

Most tools default to the http method name. To refine further would be a search by tag. Closed set of choices are important for interop, too. For example, here's the guidance of names for Google StackDriver Trace. an open-ended name would quickly overrun 128 bytes

Name of the span. Must be less than 128 bytes. The span name is sanitized and displayed in the Stackdriver Trace tool in the Google Cloud Platform Console. The name may be a method name or some other per-call site name. For the same executable and the same call point, a best practice is to use a consistent name, which makes it easier to correlate cross-trace spans.

In most discussions, where the http method isn't used (ps http method is almost always used as a default), a URI template is preferred. Depending on how many chars you have, you could still prefix by http method (helpful in post vs get). Bear in mind the implicit lowercasing.

"get /things/{thingId}"

ps I remain unconvinced the http prefix is helpful, as I think that's more a UI hack than a good naming convention. It could be misleading in https as well. Maybe that's why others don't use it. IMHO, it would be better to raise an issue on the UI to allow an overlay that shows which spans are http than hack the name.

@codefromthecrypt
Copy link
Contributor

also PS, what I mentioned above is where there's no good name specified by the user. a naming annotation or otherwise mapping would be the best option for good names.

@marcingrzejszczak
Copy link
Contributor

We had a chat about this with @adriancole and the amount of work needed to be put here is far greater than the revenue coming out of it (at least for now). We're putting this task to icebox.

@s-spindler
Copy link
Author

Thanks for the update!

BTW: the same happens with outgoing HTTP requests as the AbstractTraceHttpRequestInterceptor creates span names in the same way.

We disabled client tracing for now (setting spring.sleuth.web.client.enabled and spring.sleuth.web.async.client.enabled to false).

Regarding the TraceFilter: as a quick workaround we disabled it like this. That way we still get the spans for the controller methods as created by TraceHandlerInterceptor.

@marcingrzejszczak
Copy link
Contributor

Yeah we name the spans consistently so no wonder.

Cool! Glad to know that you've found a workaround :)

@marcingrzejszczak
Copy link
Contributor

I'm closing this for now and we'll reopen it if necessary.

@thyzzv
Copy link

thyzzv commented Feb 23, 2017

I would also like to pitch in on this discussion.
We are running into an issue when using Feign and Sleuth, that (due to a very long path variable) the X-Span-Name header is so big that the receiving-end rejects the request (400 bad-request). While when I manually execute the request without the X-Span-Name header the request executes fine.

Is this something to consider in this issue, or should I open a new one?

@marcingrzejszczak
Copy link
Contributor

Yeah that sounds like a different issue. Please provide more details there and let's try to fix it!

@ouaibsky
Copy link

ouaibsky commented Apr 1, 2017

Hi

Is it possible to reopen this question ?
As we try to build endpoints as much as possible closed to rest principle, we end up with an infinite number of different uri (containing many path variable).

The pb is zipkin ui is working more than slowly when it tries to load all span for a component leading us to not using this nice tools whereas it provide really added value.

I put in place @s-spindler workaround but the issue is some controller method are marked as cacheable and do not appear inside zipkin ui (we configure sleuth to sample nothing)

I know this is not an obvious fix, if someone got an idea on how to implement it I can think about that.

Christophe.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Apr 2, 2017 via email

@marcingrzejszczak
Copy link
Contributor

Until then I might have an idea...

You could create a wrapper over ZipkinSpanListener. Before calling the report method you could create a new span basing on the passed span to the report method. You could use the SpanBuilder to alter then name of the span too in whatever way you need.

Does it make any sense? I'm not sure if it will work as you expect but most likely it can be treated as some workaround until we figure out sth better.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Apr 2, 2017 via email

@tmack8001
Copy link

tmack8001 commented Feb 5, 2018

I would also like to have this conversation to find a common solution to this problem for many people... Is there a suggested way to have smaller values for Sleuth created spans that are more aligned with the OpenZipkin project view of span.names being reused for ease of collection and exposure to tracing UI tools?

Update: ignore my comment as I see the direction change in 2.0.x is going to answer my issues and I can either live with a temporary workaround for now or wait until this is ready for primetime.

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

No branches or pull requests

6 participants