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

Example to show how to achieve more consistent sampling across linked traces #4346

Merged
merged 21 commits into from
Apr 21, 2023

Conversation

kalyanaj
Copy link
Contributor

@kalyanaj kalyanaj commented Mar 29, 2023

This is an example to show how to get more consistent sampling even across linked traces. Please see tradeoffs listed in README.md for when this approach can be helpful and when it won't help with consistent sampling across linked traces. This uses a composite sampler that includes a parent based sampler and a links based sampler.

Fixes #4345
Design discussion issue #

Changes

This shows the example of a composite sampler that has:

  1. A parent based sampler.
  2. A links based sampler.

This composite sampler first delegates to the parent based sampler. If the parent based sampler decides to sample, then it decides to sample. However, if the parent based sampler decides to drop, the composite sampler delegates to the links based sampler. The links based sampler decides to sample if the activity has any linked activities and if at least ONE of those linked activities is sampled.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@kalyanaj kalyanaj requested a review from a team March 29, 2023 20:57
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

I've left some non-blocking comments.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #4346 (8d69bcb) into main (678e9c4) will increase coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4346      +/-   ##
==========================================
+ Coverage   84.73%   84.78%   +0.04%     
==========================================
  Files         300      300              
  Lines       12064    12064              
==========================================
+ Hits        10223    10229       +6     
+ Misses       1841     1835       -6     

see 4 files with indirect coverage changes

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I added two small nits, but no blocker for merging this.

docs/trace/links-based-sampler/README.md Outdated Show resolved Hide resolved
@kalyanaj kalyanaj changed the title An example of a composite sampler that includes a links based sampler Example to show how to achieve more consistent sampling across linked traces Mar 29, 2023
@kalyanaj
Copy link
Contributor Author

@jmacd FYI on this PR (would love to get your review/feedback on this).

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Apr 6, 2023
Copy link

@oertl oertl left a comment

Choose a reason for hiding this comment

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

This proposal would also fit with consistent probability sampling, if the composite sampler delegates to a ParentConsistentProbabilityBased sampler and an Always-on consistent probability sampler depending on whether there are linked activities.

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Apr 7, 2023
@kalyanaj
Copy link
Contributor Author

This proposal would also fit with consistent probability sampling, if the composite sampler delegates to a ParentConsistentProbabilityBased sampler and an Always-on consistent probability sampler depending on whether there are linked activities.

Thanks @oertl . Agree it would fit with it. However, since the consistent probability sampling spec is still experimental, wanted to scope this example to the current stable parent based sampling mechanism. In the future, once the consistent probability sampling mechanism becomes stable, we can update this to reflect how this would work with consistent probability sampling. Please let me know if you feel differently. Thanks!

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

I have left some non-blocking suggestions.

kalyanaj and others added 2 commits April 18, 2023 10:50
Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com>
@kalyanaj
Copy link
Contributor Author

@jmacd FYI in case you want to review this as well.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@jarias-lfx
Copy link

/easycla

@utpilla utpilla merged commit 58d9d69 into open-telemetry:main Apr 21, 2023
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.

Provide an example for how sampling based on the context of the activity links could work in OTel.NET
7 participants