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

AWS X-Ray Trace Context Propagator #202

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

awiede
Copy link

@awiede awiede commented Aug 30, 2020

Create Trace Context Propagator for AWS X-Ray. Collect Root TraceId, Parent SegmentId and Sampling decision in new SpanContext. Will add to /examples later.

@awiede awiede requested a review from a team August 30, 2020 17:55
Create Trace Context Propagator for AWS X-Ray. Collect Root TraceId,
Parent SegmentId and Sampling decision in new SpanContext. Additional
metadata fields are collected but not yet used since there isn't any way
to pass them through SpanContext yet.
@awiede awiede force-pushed the feature/xray-propagator branch from ae233da to 286a2f1 Compare August 30, 2020 18:36
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Looks neat! Thanks! Have a few suggestions but overall looks good.

_private: (),
}

impl XrayTraceContextPropagator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to have a new method

Copy link
Author

Choose a reason for hiding this comment

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

✔️

if span_context.is_valid() {
let xray_trace_id: XrayTraceId = span_context.trace_id().into();
let mut components: Vec<String> =
vec![format!("{}={}", HEADER_ROOT_KEY, xray_trace_id.0)];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably use vec![...; 3] to avoid possible reallocation

Copy link
Author

Choose a reason for hiding this comment

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

So specifying vec![...; 3] put in three copies of the first element, refactored to use Vec::with_capacity instead.

Copy link
Member

Choose a reason for hiding this comment

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

As the formatted string is always the same, you could do one allocation injector.set(AWS_XRAY_TRACE_HEADER, format!("{}={};{}={};{}={}")) to do one string allocation rather than 4 strings and a vec?

Copy link
Author

Choose a reason for hiding this comment

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

Good call @jtescher, updated. Originally I had this iterating through all k/v pairs to handle arbitrary key/value pairs in the header. The X-Ray SDK lets you pass any additional k/v pairs through, but there isn't a way to capture anything other than traceId, parent ID, and decision in the current SpanContext I don't think? Is that part of the thought with #195?

Copy link
Member

Choose a reason for hiding this comment

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

Yep #195 should get support for that.

@awiede
Copy link
Author

awiede commented Aug 31, 2020

Thanks for the 👀 @TommyCpp! Made the suggested updates

@awiede awiede requested a review from TommyCpp August 31, 2020 13:26
@awiede awiede force-pushed the feature/xray-propagator branch from 61f1959 to 7308af3 Compare August 31, 2020 14:03
* Update component vector to initialize with capacity of 3
* Create a "new" method for XrayTraceContextPropagator.
* Use SpanId.to_hex() to build hex ID
@awiede awiede force-pushed the feature/xray-propagator branch from 7308af3 to d88506c Compare August 31, 2020 18:14
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

LGTM @TommyCpp any other comments?

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM. Great work. Thanks!

@jtescher jtescher merged commit 812bc1b into open-telemetry:master Aug 31, 2020
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.

3 participants