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

bugfix aligning service names #32

Merged
merged 1 commit into from
Sep 7, 2014

Conversation

K-Jo
Copy link
Contributor

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

There is a difference in the substraction of service names in between implementations. If a service "a" gets called without span information in the headers, the brave pre processor would name the service "/a" because of the javadocs: http://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest.html#getContextPath(). The client as a counterpart takes a subpart https://github.com/kristofa/brave/blob/master/brave-client/src/main/java/com/github/kristofa/brave/client/ClientRequestInterceptor.java getServiceName

@kristofa
Copy link
Member

kristofa commented Sep 7, 2014

Hey, Indeed service names for client and server endpoints should be same. See pull request #19 .
Now we can have inconsistencies between implementations (RestEasy, Jersey,...) because on server side each implementation has its own logic for submitting end point and specifying service name. If there would be a brave-server module similar as has been done with client, see #27 , this would get aligned and we would avoid code duplication.

But your change should fix RestEasy implementation. Thanks!

kristofa added a commit that referenced this pull request Sep 7, 2014
@kristofa kristofa merged commit 26dd6e4 into openzipkin:master Sep 7, 2014
@kristofa
Copy link
Member

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

@K-Jo K-Jo deleted the service_name_alignment branch September 25, 2014 10:48
@kristofa kristofa mentioned this pull request Jan 18, 2015
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.

2 participants