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

first implementation of span name filters #33

Merged
merged 2 commits into from
Sep 12, 2014

Conversation

K-Jo
Copy link
Contributor

@K-Jo K-Jo commented Sep 2, 2014

screen shot 2014-09-02 at 10 48 05

When the path contains UUID, or Long values, the list of span names explode, In production we see that cassandra is struggling when a huge amount of different span names needs to be inserted into the database.
I created the possibility to add a filter to the span name creation. For the spring rest-easy you can add them by simply creating a config class in your project. Jersey implementation currently takes no filter. Two filters are implemented: No filter and Filter out numeric values which filters path/ab1234/path into path//path. Of course a custom filter for your application can be made.

@kristofa
Copy link
Member

kristofa commented Sep 7, 2014

I knew from the start that simply retrieving the path from the URI and use that as span name would not work when wanting to do aggregation because we would not be able to see that /service/a/1 and /service/b/2 are requests to same service. I didn't expect it would cause issues at datastore level but I can see the issue! So thanks for taking this up!

However I see 1 issue with the implementation:

  • When we change the span name on the client side we should make sure the same span name is used at the receiving end (server side). I have already foreseen a header for this X-B3-SpanName which is submitted when you call the ClientRequestHeaders#addTracingHeaders method which takes the spanName as last argument. So if your filter adapts the span name you should submit it by using the right call to ClientRequestHeaders#addTracingHeaders. Interpretation of the header is implemented at server side already.

As a side note, defining service name and span name feels like more complicated as it should :) I'll try to get my head around it one day when rainy and/or cold :) to see if we can't make it easier.

@K-Jo
Copy link
Contributor Author

K-Jo commented Sep 9, 2014

Hey Kristof,

I updated the code so it indeed adds the span name to the request headers and added tests. I took the liberty to remove the method in ClientRequestHeaders which didn't add the span name. The method wasn't used anymore except for testing, which I adapted.

Pieter

@kristofa
Copy link
Member

Ok, will merge. Thanks.

kristofa added a commit that referenced this pull request Sep 12, 2014
First implementation of span name filters. Those filters are implemented mainly to filter variable content from urls  and so avoid we end up with lots and lots of different span names.
@kristofa kristofa merged commit b121d4c into openzipkin:master Sep 12, 2014
@kristofa
Copy link
Member

This pull request made it into brave version 2.3 which should appear in Maven Central shortly.

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 this pull request may close these issues.

None yet

2 participants