-
Notifications
You must be signed in to change notification settings - Fork 902
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
distributed tracing for armeria #114
Conversation
w00t! |
@trustin Please take a look |
<dependency> | ||
<groupId>com.github.kristofa</groupId> | ||
<artifactId>brave-http</artifactId> | ||
<version>3.4.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a property instead of duplicating version numbers?
@synk First pass done. Please ping me for another pass. |
@trustin Thanks. I'll ping when I finished |
@trustin I think I handled your comments. Please take a look again. |
/** | ||
* Creates a new tracing {@link Client} decorator using the specified {@link Brave} instance. | ||
*/ | ||
public static Function<Client, Client> decorator(Brave brave) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming this method to newDecorator()
for consistency with MetricCollectingClient.new...Decorator()
?
Please fix the Javadoc errors:
It seems like Javadoc does not like |
@synk Looks good to me except for a few nits. Please squash for a merge. |
@trustin I've squashed. Please merge if it's OK. |
* Returns server side annotations that should be added to span. | ||
*/ | ||
protected <T> List<KeyValueAnnotation> annotations(ServiceInvocationContext ctx, Future<? super T> result) { | ||
final List<KeyValueAnnotation> annotations = new ArrayList<>(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot setting capacity here.
Introduces distributed tracing capability to armeria clients/servers. This enables us to watch timing data among the whole armeria rpc chain. This feature depends on brave distributed tracing library compatible with zipkin.
distributed tracing for armeria
BOOM! |
Introduces distributed tracing capability to armeria clients/servers. This enables us to watch timing data among the whole armeria rpc chain.
This feature depends on brave distributed tracing library compatible with zipkin.
Decorate HTTP client
Decorate HTTP server
Add custom annotations using brave's API