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

Use OpenTelemetry API #51

Merged
merged 10 commits into from
Jan 14, 2020
Merged

Use OpenTelemetry API #51

merged 10 commits into from
Jan 14, 2020

Conversation

trask
Copy link
Member

@trask trask commented Jan 8, 2020

🎉🎉🎉

Some instrumentation was commented out in this PR, to be brought back later (tracking in #50).

Closes #43

@trask
Copy link
Member Author

trask commented Jan 11, 2020

I discussed with @tylerbenson, I'm going to break this PR up into smaller commits (still one PR). Unfortunately, I'm not sure if it can be broken into smaller commits that are individually compilable/testable, but at least this will make the PR easier to review (and we squash down to a single commit during merge anyways).

@trask
Copy link
Member Author

trask commented Jan 13, 2020

Now broken up into smaller tests. Muzzle build is failing due to recently released couchbase java-client 3.0.0 (not due to changes in this PR).

@tylerbenson
Copy link
Member

@trask you can pull this over if you like: DataDog/dd-trace-java#1170

for (final String key : getter.keys(carrier)) {
// extracted header value
String s = getter.get(carrier, key);
// in case of multiple values in the header, need to parse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if OpenTelemetry handles this edge case already?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but I'm not sure that it should? (e.g. Tracestate header value is comma-separated key-value pairs)

if (span == null) {
return;
}
span = span.getLocalRootSpan();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to cause some weirdness... I couldn't think of a better solution though. Ideas?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of anything better either. I just added this to #50 so we can come back to it later.

@trask trask merged commit 64fd998 into open-telemetry:master Jan 14, 2020
@trask trask deleted the use-opentelemetry-api branch January 14, 2020 00:51
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.

Change instrumentation to use OpenTelemetry API (indirectly)
2 participants