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.

Sign up for GitHub

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

Jump to bottom

feat: Add DeterministicSampler for easier sampling #70

Merged
merged 10 commits into from
Feb 13, 2024
Merged

feat: Add DeterministicSampler for easier sampling #70

merged 10 commits into from
Feb 13, 2024

Conversation

JamieDanielson
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • add Deterministic Sampler
  • add sampleRate info to debug
  • add unit tests and update smoke test

How to verify that this has the expected result

  const sdk = new HoneycombWebSDK({
    apiKey: HONEYCOMB_API_KEY,
    serviceName: 'web-distro',
    debug: true,
    sampleRate: 2,
    instrumentations: [getWebAutoInstrumentations()], // add auto-instrumentation
  });

With a sampleRate higher than one you may have to generate a few traces before it shows up in Honeycomb! But with debug enabled at least you can see the config logged, and also see the "Non recording span" if it's not going to send because it's being dropped with sampling.

@JamieDanielson JamieDanielson requested review from a team as code owners February 12, 2024 23:25
@JamieDanielson JamieDanielson self-assigned this Feb 12, 2024
@JamieDanielson JamieDanielson added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Feb 12, 2024
kentquirk
kentquirk approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

A couple of minor nits, but no blockers.

src/deterministic-sampler.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
test/deterministic-sampler.test.ts Outdated Show resolved Hide resolved
ahrbnsn
ahrbnsn reviewed Feb 13, 2024
View reviewed changes
src/util.ts Outdated Show resolved Hide resolved
ahrbnsn
ahrbnsn approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@ahrbnsn ahrbnsn left a comment

Choose a reason for hiding this comment

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

🎉

@JamieDanielson JamieDanielson merged commit 5318210 into main Feb 13, 2024
8 checks passed
@JamieDanielson JamieDanielson deleted the jamie.sampler branch February 13, 2024 23:33
@JamieDanielson JamieDanielson mentioned this pull request Feb 14, 2024
Merged
JamieDanielson added a commit that referenced this pull request Feb 14, 2024
@JamieDanielson
feat: add AlwaysOffSampler (#72)
3253490 
## Which problem is this PR solving?

- #70 added deterministic sampling but converted a sample rate of 0 to
1, but it may be nice to have the option keep the AlwaysOffSampler as an
option (we have this option in the node distro).

## Short description of the changes

- Allow sample rate of 0 and don't convert to default of 1
- Add AlwaysOffSampler to be used with sample rate of 0; drop
everything.
- Update tests

## How to verify that this has the expected result

Setting a sample rate of 0 is valid and should drop all telemetry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrbnsn ahrbnsn ahrbnsn approved these changes

@kentquirk kentquirk kentquirk approved these changes

Assignees

@JamieDanielson JamieDanielson

Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feat] Add DeterministicSampler for easier sampling
3 participants
@JamieDanielson @kentquirk @ahrbnsn