-
Notifications
You must be signed in to change notification settings - Fork 104
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 test for child span sampling issue #264
Add test for child span sampling issue #264
Conversation
Codecov Report
@@ Coverage Diff @@
## main #264 +/- ##
==========================================
+ Coverage 36.39% 36.42% +0.03%
==========================================
Files 37 37
Lines 3168 3168
==========================================
+ Hits 1153 1154 +1
+ Misses 2015 2014 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The reason why it's returning |
Anyways, I found the bug, the parent based sampler pattern matched on |
@@ -135,12 +135,11 @@ parent_based_sampler(#span_ctx{trace_flags=TraceFlags, | |||
parent_based_sampler(#span_ctx{is_remote=true}, #{remote_parent_not_sampled := SamplerAndOpts}) -> | |||
SamplerAndOpts; | |||
%% local parent sampled | |||
parent_based_sampler(#span_ctx{trace_flags=TraceFlags, | |||
is_remote=false}, #{local_parent_sampled := SamplerAndOpts}) | |||
parent_based_sampler(#span_ctx{trace_flags=TraceFlags}, #{local_parent_sampled := SamplerAndOpts}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now this is returning the local_parent_sampled
sampler but not verifying that the span is not remote?
Is the issue instead that is_remote
should be being set to false instead of undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not because is_remote=true
was pattern matched above. I have now changed the fix by adding is_remote=false
in root_span_ctx()
, not sure if this is the only place. The typespec by the way is still boolean() | undefined
, not sure if you want to leave it like that or search also for other places and make it boolean()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is get_ctx
in otel_span_ets
, which is another potential candidate for setting is_remote=false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got to check the spec this morning. This is likely a result of the multiple iterations done in the spec on how remote spans were created and stored. At one point they were stored in a separate context key from the current span context key and then I think that was switched to a remote field, but it has been so long I need to refamiliarize myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, now its coming back to me. The is remote was not needed also when using what are called "nonrecording spans" for the extracted remote spans. But the spec does still include is_remote
. I'm not 100% if is_remote=true
can simply be set in non_recording_span/3
or if it needs to be done in the propagator itself... But either way, one of those two are the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah in that case the initial fix (not setting is_remote=true
but implicitly checking for false
or undefined
) makes more sense to me. We can also change it to explicitly check for false
and undefined
? Let me know what you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my PR that I think will fix it. I went with defaulting is_remote
to false
#265
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Do you want to keep the Elixir test in this PR or make a erlang test in #265 and can this PR be closed?
The test is good to keep, just need the Can you also rebase this down to 1 commit instead of 4? |
f8ed876
to
a1cd744
Compare
Done :) |
No description provided.