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

Adds OTelTraceIdRatioBasedSampler #131

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Adds OTelTraceIdRatioBasedSampler #131

wants to merge 8 commits into from

Conversation

t089
Copy link

@t089 t089 commented Aug 12, 2024

This adds a OTelTraceIdRatioBasedSampler (see Spec).

The sampler takes the last 8 bytes of the TraceID and interprets them as a single UInt64 value which is then used for the sampling decision. As long as those bytes are randomly distributed, this gives a deterministic, but random sampling decision.

This also adds the Benchmark package to benchmark the implementation.

At the moment, this draft makes use of a proposed breaking change in swift-w3c-trace-context to avoid an allocation when accessing the bytes or TraceID: slashmo/swift-w3c-trace-context#18

Package.swift Outdated
.package(url: "https://github.com/slashmo/swift-w3c-trace-context.git", from: "1.0.0-beta.1"),
//.package(url: "https://github.com/slashmo/swift-w3c-trace-context.git", from: "1.0.0-beta.1"),
//.package(name: "swift-w3c-trace-context", path: "../swift-w3c-trace-context"),
.package(url: "https://github.com/t089/swift-w3c-trace-context.git", branch: "trace-id-bytes"),
Copy link
Author

Choose a reason for hiding this comment

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

Please see slashmo/swift-w3c-trace-context#18 for details on this

Copy link
Owner

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🙏 It's a good addition to the package, thanks for also adding benchmarks 👍 I'll do a final review pass once we get the breaking change in W3C Trace Context merged. My comments so far have mostly been around formatting.

@slashmo slashmo marked this pull request as ready for review September 29, 2024 10:34
@slashmo slashmo marked this pull request as draft September 29, 2024 10:34
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (90d63b4) to head (efb467d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131       /-   ##
==========================================
  Coverage   99.00%   99.01%    0.01%     
==========================================
  Files          65       68        3     
  Lines        1800     1831       31     
==========================================
  Hits         1782     1813       31     
  Misses         18       18              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t089 t089 changed the title Draft: Adds OTelTraceIdRatioBasedSampler Adds OTelTraceIdRatioBasedSampler Oct 2, 2024
@t089 t089 marked this pull request as ready for review October 2, 2024 08:05
@t089 t089 requested a review from slashmo October 2, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add trace-id ratio based sampler (probabilistic)
2 participants