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

feat(dynamic_samping): Adjust sample rate by envelope header's sample_rate [INGEST-1395] #1327

Merged
merged 31 commits into from
Jul 4, 2022

Conversation

untitaker
Copy link
Member

@untitaker untitaker commented Jun 30, 2022

We have a new envelope header that tells us the configured sample rate of the client that initiated the trace. Adjust trace sampling by that incoming sample rate.

TODO:

  • adjust transaction sample rate as well

@untitaker untitaker requested a review from a team June 30, 2022 11:01
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
"user_id": "hello"
}
"###);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we test this with some more values for sample_rate? E.g. "bogus", "10e4", ...

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a test for a negative sample_rate?

relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-common/src/serde.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-general/Cargo.toml Outdated Show resolved Hide resolved
relay-server/src/actors/envelopes.rs Outdated Show resolved Hide resolved
@untitaker
Copy link
Member Author

untitaker commented Jun 30, 2022

@jan-auer okay, I addressed half of the review comments. I am not acting on two things in this PR:

  • Sampling readjustment in case of relay chains. I looked into implementing this as we discussed offline and I now believe this actually is not possible at the moment without a larger refactor that merges trace-sampling and event-sampling into one function. That's a refactor I want to do anyway. But after this PR is merged.
  • Failing attributes individually. If we want to do this we should do it for all attributes or have a real policy about this. Right now we don't, and throw the entire sampling context away. And IMO that's not even that wrong, after all we should probably not apply dynamic sampling at all if we can't parse basic attributes. We could however store the raw value in the ErrorBoundary and use it to serialize the value again.

I think the PR as-is gets us quite far already. It doesn't make the existing problem of double-sampling worse, and we can run demos in prod as long as we point SDKs to the apex domain.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Requires the clippy fix in #1329.

relay-common/src/serde.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Blocking for panicking division by 0.

relay-general/src/protocol/contexts.rs Outdated Show resolved Hide resolved
relay-server/src/actors/envelopes.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
"user_id": "hello"
}
"###);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a test for a negative sample_rate?

Co-authored-by: Iker Barriocanal <[email protected]>
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Removing my blocking, division by 0 doesn't panic with floats.

@iker-barriocanal iker-barriocanal dismissed their stale review July 1, 2022 15:13

Removing my blocker, division by 0 doesn't panic with floats.

relay-common/Cargo.toml Outdated Show resolved Hide resolved
relay-general/src/store/mod.rs Outdated Show resolved Hide resolved
relay-general/src/store/normalize.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
relay-sampling/src/lib.rs Show resolved Hide resolved
relay-sampling/src/lib.rs Show resolved Hide resolved
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.

4 participants