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

Feature core annotations #1

Merged
merged 1 commit into from May 31, 2016
Merged

Conversation

@vears91
Copy link

vears91 commented May 25, 2016

Support for Zipkin's core annotations, described in the Thrift specification and the instrumentation documentation

@alimaredia

This comment has been minimized.

Copy link

alimaredia commented May 26, 2016

@cbodley just wanted to give you the heads up about this PR

endpoint->name, endpoint->port, endpoint->ip,
trace->info.trace_id, trace->info.span_id,
trace->info.parent_span_id,
annotation->val_str);

This comment has been minimized.

Copy link
@cbodley

cbodley May 28, 2016

careful with the whitespace changes, some of these extra tabs don't look right

CLIENT_RECV_FRAGMENT,
SERVER_SEND_FRAGMENT,
SERVER_RECV_FRAGMENT
} blkin_core_annotation;

This comment has been minimized.

Copy link
@cbodley

cbodley May 28, 2016

i like the way that openzipkin does these annotations, i.e. const string CLIENT_SEND = "cs". i'd prefer that we use a string representation too, so that we don't need the extra step to map enum values into strings with get_core_annotation_value(). could you change these to string constants?

const char* const CLIENT_SEND = "cs";
const char* const CLIENT_RECV = "cr";
...
@cbodley

This comment has been minimized.

Copy link

cbodley commented May 31, 2016

@vh4x thanks! looking at the 'Files changed' tab, i still see a lot of unrelated whitespace changes. could you please clean those up and squash everything into a single commit?

@vears91 vears91 force-pushed the vears91:feature-core-annotations branch from c34958e to 3a77251 May 31, 2016
@vears91

This comment has been minimized.

Copy link
Author

vears91 commented May 31, 2016

Now I get what you mean. I squashed the previous commits, if something's still missing let me know

@cbodley cbodley merged commit 35388dc into linuxbox2:master May 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.