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

NH-11236 sampler creates Service Entry Internal KVs #12

Merged

Conversation

tammy-baylis-swi
Copy link
Contributor

Here the custom Sampler creates Service Entry Internal KVs by setting them as attributes only for the root span:

  1. BucketCapacity
  2. BucketRate
  3. SampleRate
  4. SampleSource

I've also tidied the calculate_attributes a little bit to avoid ambiguity in new attributes assignment.

I've done some manual testing, with resulting exported traces with those KVs recorded on this doc: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2893414559/2022-04-13+service+entry+internal+KVs+testing It doesn't really cover the different SampleSource results possible though. Would that be liboboe scope or in this PR's scope?

This PR is to merge into add-custom-propagator branch at the moment because this one hasn't been merged yet #11 But I can change the destination if this one needs lots of work too! Please let me know and/or if I need to do more testing.

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tammy-baylis-swi! I think this is ready w/o further SampleSource testing since that's all implemented by liboboe. In fact the test results you provided has helped highlight a bug in liboboe -- the returned SampleSource and SampleRate should be -1 for trigger traces per spec, but liboboe is setting to 0. I'll submit a PR to get that fixed :)

@tammy-baylis-swi
Copy link
Contributor Author

Hi @cheempz ! Based on our separate chat, in 23a12a2 I've updated the Sampler's calculate_attributes method to set the service entry KVs (bucket/sample) only if liboboe returns non -1 values -- instead of every root span as before. I also merged your liboboe updates into my add-otel-oboe-features branch and rebuilt. Test traces are are on the lower half of the table on this doc, including a test of Acceptance 5 to double-check downstream of new decision-making: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2893414559/2022-04-13+to+18+service+entry+internal+KVs+testing#At-23a12a2-and-liboboe-merge%3A

I wasn't sure if the identification of a continued decision should be based on all four liboboe outputs. Currently, if even one is not -1 then it is considered not a continued trace. If all four are -1 then it is a continued trace. That helper is here: https://github.com/appoptics/opentelemetry-python-instrumentation-custom-distro/blob/23a12a255f8d0e159b5726cc3da7bb710756b216/opentelemetry_distro_solarwinds/sampler.py#L128-L141

Please let me know your thoughts! In the meantime I'll try to resolve the merge conflicts. I made some commits to the big PR this morning which ended up refactoring calculate_attributes quite a bit... I think I did them in the wrong order 😅

@cheempz cheempz self-requested a review April 22, 2022 16:21
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tammy-baylis-swi! Theoretically if sample rate and bucket rate are -1 it would indicate a continued decision, but I think checking all four decision values is the most correct and accessing two more dictionary shouldn't cost that much ;)

@tammy-baylis-swi tammy-baylis-swi merged commit 68a0e63 into add-custom-propagator Apr 22, 2022
@tammy-baylis-swi tammy-baylis-swi deleted the NH-11236-service-entry-internal-kvs branch April 22, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants