Add SSS tests to cover a parent-child spans case#848
Merged
Conversation
katiehockman
reviewed
Feb 7, 2023
katiehockman
left a comment
katiehockman
left a comment
Contributor
There was a problem hiding this comment.
Thanks! Just some small nits
Co-authored-by: Katie Hockman <[email protected]>
Co-authored-by: Katie Hockman <[email protected]>
ZStriker19
reviewed
Feb 7, 2023
pierotibou
reviewed
Feb 7, 2023
Qard
reviewed
Feb 8, 2023
3 tasks
Qard
approved these changes
Feb 9, 2023
|
Looks like the linter has some issues, but other than that LGTM. |
bantonsson
reviewed
Feb 9, 2023
ygree
force-pushed
the
ygree/parent-child-SSS-tests
branch
from
February 9, 2023 16:08
5977520 to
476caf6
Compare
katiehockman
approved these changes
Feb 9, 2023
Kyle-Verhoog
approved these changes
Feb 9, 2023
Kyle-Verhoog
left a comment
Kyle-Verhoog
left a comment
Member
There was a problem hiding this comment.
lgtm from a parametric/code quality standpoint
Qard
approved these changes
Feb 9, 2023
robertomonteromiguel
approved these changes
Feb 10, 2023
Contributor
|
I'll propose additions to this test in a separate PR. This LGTM. |
|
Seems to be a bunch of dotnet and java failures, but doesn't look to be related. Can we land this? |
Collaborator
Yes, you can ignore them |
Hellzy
pushed a commit
that referenced
this pull request
Feb 22, 2023
* Add SSS tests to cover a parent-child spans case * Update parametric/test_span_sampling.py Co-authored-by: Katie Hockman <[email protected]> * Update parametric/test_span_sampling.py Co-authored-by: Katie Hockman <[email protected]> * fix parametric tests * remove comment * Assert the parent span trace priority only. * Fix misleading comment * Bump Java Tracer to 1.8.0 to pass the tests --------- Co-authored-by: Katie Hockman <[email protected]> Co-authored-by: Katie Hockman <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A couple more tests to verify SSS when a parent/child span kept by SSS but the other is dropped by the tracer sampling rules.
The corresponding fix PR for Java Tracer
incident-18687
I also ran
test_root_span_selected_by_sss014for tracers other than Java. Go, .NET and Python all set_sampling_priority_v1to-1for the root span (should be2according to the RFC). Update: this sampling priority assertion wasn't relevant as the Agent handles this properly anywayAlso they don't set
_dd.p.dmfor the sampled span. Update: single spans should have this tag in this case, the specification was modified to clarify thisNodeJS: fails with
InactiveRpcErrorCheck list
Your PR is not ready to be reviewed? Please save it as a draft 🙏
Yes to all? Feel free to merge it whenever you want ❤️