Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix(sampling): Sample transactions without DSC #1667

Merged
merged 9 commits into from
Dec 5, 2022
Merged

fix(sampling): Sample transactions without DSC #1667

merged 9 commits into from
Dec 5, 2022

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Dec 3, 2022 •
edited
Loading

Dynamic sampling relies on the Dynamic Sampling Context (DSC). The envelope header contains information about the trace, the origin project, and attributes for trace sampling.

Originally, when the DSC is not specified in an envelope, dynamic sampling did not run on the envelope. Transaction events in this envelope are effectively sampled at 100% percent. However, there are more cases where the DSC is not available or not valid:

  • Old or incompatible SDKs do not send a DSC, but the transaction needs to be sampled at least with the uniform sample rate.
  • The DSN public key in the DSC could point to a nonexistent project or a project from another organization.
  • There could be an error resolving the project state for the specified project.

This PR makes the following changes:

  • Uses the event to generate an ad-hoc DSC for trace sampling if there's no DSC in the event or the sampling project state cannot be retrieved for any reason.
  • Ensures that only rules from the same organization are used for trace sampling, falling back to the local project.

@jan-auer jan-auer self-assigned this Dec 3, 2022
@jan-auer jan-auer added this to the Maintenance milestone Dec 5, 2022
jjbayer
jjbayer approved these changes Dec 5, 2022
View reviewed changes
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

This looks good to me, do we need to add a test to cover the new behavior?

relay-server/src/actors/project.rs
if let Some(state) = self.valid_state() {
// Never use rules from another organization.
if state.organization_id == message.project_state.organization_id {
message.sampling_project_state = Some(state);
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 log a statsd metric to know how often this happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about this. Are we interested in how often this happens or how many projects this happens with? I haven't found a good way to log something truly useful.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah maybe add the project ID / org ID to a set metric?

@jan-auer
Merge branch 'master' into fix/ds-without-dsc
ffabbca 
* master:
  ref(server): Remove internal dynamic sampling metrics (#1671)
  fix(build): Pin ubuntu image to 20.04 (#1672)
  ref(ffi,cabi): Replace outdated failure with anyhow crate (#1669)
@jan-auer jan-auer marked this pull request as ready for review December 5, 2022 15:28
@jan-auer jan-auer requested a review from a team December 5, 2022 15:28
@jan-auer jan-auer enabled auto-merge (squash) December 5, 2022 15:40
@jan-auer jan-auer merged commit d9f92a8 into master Dec 5, 2022
@jan-auer jan-auer deleted the fix/ds-without-dsc branch December 5, 2022 15:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jjbayer jjbayer jjbayer approved these changes

Assignees

@jan-auer jan-auer

Labels
None yet
Projects
None yet
Milestone
Maintenance
Development

Successfully merging this pull request may close these issues.
2 participants
@jan-auer @jjbayer