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

Add AWS X-Ray Propagator #1300

Closed
wants to merge 16 commits into from
Closed

Add AWS X-Ray Propagator #1300

wants to merge 16 commits into from

Conversation

lupengamzn
Copy link
Contributor

Changes

Added support for propagating AWS X-Ray trace header.

@lupengamzn lupengamzn requested a review from a team September 24, 2020 01:39
@lupengamzn lupengamzn requested a review from anuraaga September 24, 2020 21:12
Copy link

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #1300 into master will increase coverage by 0.73%.
The diff coverage is 79.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1300      +/-   ##
==========================================
+ Coverage   77.88%   78.62%   +0.73%     
==========================================
  Files         226      220       -6     
  Lines        6386     6387       +1     
==========================================
+ Hits         4974     5022      +48     
+ Misses       1412     1365      -47     
Impacted Files Coverage Δ
...metry.Api/Context/Propagation/AWSXRayPropagator.cs 79.50% <79.50%> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 75.00% <0.00%> (-8.34%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
src/OpenTelemetry/Trace/ActivitySourceAdapter.cs 92.42% <0.00%> (-4.35%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 77.77% <0.00%> (-3.18%) ⬇️
...enTelemetry/Metrics/Export/PushMetricController.cs 86.11% <0.00%> (-1.39%) ⬇️
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 98.85% <0.00%> (-0.25%) ⬇️
...ation.SqlClient/SqlClientInstrumentationOptions.cs 97.91% <0.00%> (-0.05%) ⬇️
src/OpenTelemetry.Api/Trace/TelemetrySpan.cs 88.23% <0.00%> (ø)
src/OpenTelemetry/Internal/SelfDiagnostics.cs 0.00% <0.00%> (ø)
... and 66 more

return false;
}

var activityTraceId = ActivityTraceId.CreateFromString(traceId.AsSpan());
Copy link
Member

Choose a reason for hiding this comment

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

Lots of string manipulation above to pass a span here. If you switch the above code to manipulate spans over the string/char[] it will probably be less memory instensive/faster overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just switched from manipulating String to ReadOnlySpan in the latest version

@CodeBlanch
Copy link
Member

I know I'm risking "lost" packages here (😄), but I want to ask: Would this be better in the contrib repo? This part of the spec seems to be saying we shouldn't have vendor-specific propagators in the main repo:

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#vendor-specific-propagators

@lupengamzn
Copy link
Contributor Author

lupengamzn commented Sep 25, 2020

I know I'm risking "lost" packages here (😄), but I want to ask: Would this be better in the contrib repo? This part of the spec seems to be saying we shouldn't have vendor-specific propagators in the main repo:

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#vendor-specific-propagators

That's what we're thinking previously, but in Java seems like it's keeping all propagators in the same place and that's why we put it in the main repo. Also, as we're using interface from OT SDK, how can we do that in contrib repo in this case, suggestions would be much appreciated! :)

Update: Seems like there is an agreement about where to put the vendor-specific propagators, and turns out it should be in the main repo. Let's wait for @cijothomas 's clarification.

@cijothomas
Copy link
Member

open-telemetry/opentelemetry-specification#1043 Once this spec PR is merged, we can officially confirm that vendor specific propagators can be hosted in the main repo itself.

@lupengamzn lupengamzn requested a review from CodeBlanch October 1, 2020 23:58
@lupengamzn
Copy link
Contributor Author

Moving from main repo to contrib repo

@lupengamzn lupengamzn closed this Oct 14, 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.

5 participants