Skip to content

Conversation

@vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Jun 2, 2022

Fixes #3315

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #3329 (fb2d5a6) into main (a2aa305) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3329      +/-   ##
==========================================
- Coverage   85.44%   85.27%   -0.17%     
==========================================
  Files         270      270              
  Lines        9561     9561              
==========================================
- Hits         8169     8153      -16     
- Misses       1392     1408      +16     
Impacted Files Coverage Δ
src/OpenTelemetry/Trace/TracerProviderSdk.cs 96.40% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (-28.58%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (-18.19%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.20% <0.00%> (-4.40%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@vishweshbankwar vishweshbankwar marked this pull request as ready for review June 2, 2022 16:38
@vishweshbankwar vishweshbankwar requested a review from a team June 2, 2022 16:38
private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId traceId, bool isParentRemote)
{
var isRootSpan = traceId == default;
var isRootSpan = traceId == default || isParentRemote;
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 semantically buggy, I guess?

Maybe:

var isRootSpan = traceId == default;

return isRootSpan || isParentRemote
    ? ...


## Unreleased

* Included `isRemote` check in `PropagateOrIgnoreData` in `TracerProviderSDK` to
Copy link
Member

Choose a reason for hiding this comment

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

TracerProvider modified to create activity irrespective of SamplingResult, for spans with Remote Parent, to maintain context propagation.

^ or similar wording.

@cijothomas
Copy link
Member

@vishweshbankwar will hold off from merging for a day, as we are doing 1.3.0 stable release, and want to avoid any functional changes.

@cijothomas
Copy link
Member

@vishweshbankwar Please fix the conflict, and we are good to go


[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId traceId)
private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId traceId, bool isParentRemote)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably explore updating the method signature here to something like this:
private static ActivitySamplingResult PropagateOrIgnoreData(in ActivityContext parentContext) and pass in options.Parent to this method. ActivityContext is a readonly struct so using the in modifier might give some minor perf gains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Minor:

Method Mean Error StdDev
WithoutInParam 716.8 ns 12.69 ns 11.87 ns
WithInparam 676.5 ns 7.27 ns 6.44 ns

I have updated the method signature

@cijothomas cijothomas merged commit 5a566e6 into open-telemetry:main Jun 6, 2022
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.

Propagation in case of remote parent

4 participants