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(server): Add dynamic sampling #835

Merged
merged 35 commits into from
Nov 26, 2020
Merged

feat(server): Add dynamic sampling #835

merged 35 commits into from
Nov 26, 2020

Conversation

RaduW
Copy link
Contributor

@RaduW RaduW commented Nov 9, 2020 •
edited
Loading

Implement dynamic sampling for transactions.

@RaduW
WIP, before doing sampling conditioned on project cache
318a2e0
@RaduW
WIP add error boundary to trace context
a2bf249
@RaduW
WIP Add support for fast path transaction processing
53b68e6
@RaduW
WIP handler for SampleTransactions
d09f38e
@RaduW
WIP before plugging in SampleTransactions into HandleEnvelope
e80eeda
@RaduW
WIP change SampleTransactions from message to public function
e3fd7fc
@RaduW
WIP code complete, sill needs review
01e9ebe
@RaduW
moved dynamic_sampling to relay_server/utils
82c3ee6
@RaduW
add unit tests
6cbb9a5
@RaduW
fix f64 compare in unit tests
f2efd48
@RaduW
wip before cleaning up basic_project_config
b282144
@RaduW
Merge branch 'master' into feat/dynamic-sampling-1
eb37909 
# Conflicts:
#	relay-server/Cargo.toml
@RaduW
Merge branch 'master' into feat/dynamic-sampling-1
08c89ce
@RaduW
add change log
3eb957a
@RaduW
fix serialisation
76c9d38
@RaduW
fix sample_transaction logic
7bf9988
@RaduW
fix fast path
7081076
@RaduW
Running integration tests
2801447
@RaduW
fix formatting (lint)
45078f0
@RaduW RaduW requested a review from jan-auer November 11, 2020 09:51
@RaduW RaduW marked this pull request as ready for review November 11, 2020 15:26
@RaduW RaduW requested a review from a team November 11, 2020 15:26
@jan-auer
Merge branch 'master' into feat/dynamic-sampling-1
be36340 
* master:
  fix(tests): Fix disabled haproxy tests (#838)
  release: 20.11.0
  doc: Use automatic links (#836)
  doc: Bring back path of an event through relay (#831)
jan-auer
jan-auer requested changes Nov 17, 2020
View reviewed changes
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.

This looks good overall, and approach seems very reasonable. A number of smaller comments and change requests below.

relay-filter/src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
relay-server/src/actors/project.rs Outdated Show resolved Hide resolved
relay-server/src/actors/project.rs Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
relay-server/src/utils/error_boundary.rs Show resolved Hide resolved
jan-auer
jan-auer approved these changes Nov 23, 2020
View reviewed changes
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.

G2G once integration tests are passing.

relay-server/src/actors/events.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/endpoints/common.rs Outdated Show resolved Hide resolved
relay-server/src/actors/events.rs Outdated
@@ -1461,7 +1472,15 @@ impl Handler<HandleEnvelope> for EventManager {
None => Err(ProcessingError::RateLimited(checked.rate_limits)),
}
}))
.and_then(|envelope| {
sample_transaction(envelope, sampling_project, false).then(|result| match result {
Copy link
Member

Choose a reason for hiding this comment

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

nit: This .then can be moved to the outer future chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what are you proposing, it is already at the top level of a long future chain.

relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
@RaduW RaduW merged commit 48e89c0 into master Nov 26, 2020
@RaduW RaduW deleted the feat/dynamic-sampling-1 branch November 26, 2020 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-auer jan-auer jan-auer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RaduW @jan-auer