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

Leverage ActivityListener.AutoGenerateRootContextTraceId #1007

Merged
merged 8 commits into from Aug 5, 2020
Merged

Leverage ActivityListener.AutoGenerateRootContextTraceId #1007

merged 8 commits into from Aug 5, 2020

Conversation

cijothomas
Copy link
Member

Fixes portions of #941 #953

Changes

Leverage ActivityListener.AutoGenerateRootContextTraceId so that sampling callbacks can get the actual TraceId of the to-be-created Activity and make decision based on TraceId.
Before this PR, Samplers generated a random TraceId, and made sampling decision. But this generated TraceID was not actually used when the Activity is created, leading to broken traces, if samplers made decision based on the TraceId.

Added Tests. There are TODOs which require follow up.

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

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

  • Design discussion issue #
  • Changes in public API reviewed

@cijothomas cijothomas requested a review from a team as a code owner August 5, 2020 06:59
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #1007 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   68.91%   68.86%   -0.06%     
==========================================
  Files         220      220              
  Lines        5999     5999              
  Branches      984      983       -1     
==========================================
- Hits         4134     4131       -3     
- Misses       1596     1599       +3     
  Partials      269      269              
Impacted Files Coverage Δ
src/OpenTelemetry/Sdk.cs 89.28% <100.00%> (+1.19%) ⬆️
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs 80.00% <0.00%> (-13.34%) ⬇️

@@ -10,7 +10,7 @@
* `BatchingActivityProcessor`/`SimpleActivityProcessor` is disposable and it
disposes the containing exporter.
* `BroadcastActivityProcessor`is disposable and it disposes the processors.

* Samplers now get the actual TraceId of the Activity to be created.
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need an empty line here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your favourite tool caught it as well!

Copy link
Member

Choose a reason for hiding this comment

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

🚨

@alanwest
Copy link
Member

alanwest commented Aug 5, 2020

Just want to clarify my understanding, next OTel release will coincide with preview 8. So, will preview 8 will not contain the removal of AutoGenerateRootContextTraceId? Or is this just getting the functionality/tests in place and then the plan is to change this prior to release?

@cijothomas
Copy link
Member Author

Just want to clarify my understanding, next OTel release will coincide with preview 8. So, will preview 8 will not contain the removal of AutoGenerateRootContextTraceId? Or is this just getting the functionality/tests in place and then the plan is to change this prior to release?

Preview8 won't remove AutoGenerateRootContextTraceId. It'll be RC1 or RC2.

@cijothomas cijothomas merged commit d0e8484 into open-telemetry:master Aug 5, 2020
@cijothomas cijothomas deleted the cijothomas/samplingtraceidtobecreatedfix branch August 5, 2020 14:59
@CodeBlanch
Copy link
Member

@cijothomas @reyang Hey I just noticed something, this broke the ParentOrElse sampler.

            var parentContext = samplingParameters.ParentContext;
            if (parentContext == default)

parentContext == default will now never be true, because a TraceId is always generated. Not sure how to detect "no parent" case now. Check SpanId = all 0s?

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.

None yet

4 participants