fix(server): Forward metrics in proxy mode #3106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TBS1996
changed the title
TBS1996
changed the title
TBS1996
commented
Feb 14, 2024
relay-server/src/services/project.rs
Outdated
| if let Some(scoping) = self.partial_scoping() { | ||
| let mut bucket_map = HashMap::new(); | ||
| bucket_map.insert(scoping.project_key, buckets); | ||
| project_cache.send(FlushBuckets { |
There was a problem hiding this comment.
skipped the aggregator for simplicity because it checks for scoping from projectstate and also features in rate_limit_and_merge_buckets.
i wasnt sure if we should also skip handle_flush_buckets? it checks if it's ratelimited, do we care about ratelimiting in proxy mode?
TBS1996
commented
Feb 14, 2024
TBS1996
commented
Feb 14, 2024
TBS1996
commented
Feb 15, 2024
tests/integration/test_metrics.py
Outdated
| @@ -62,6 +62,52 @@ def metrics_by_name_group_by_project(metrics_consumer, timeout=None): | |||
| return metrics_by_project | |||
|
|
|||
|
|
|||
| def test_metrics_proxy_mode(mini_sentry, relay): | |||
There was a problem hiding this comment.
this test fails without the changes in this PR
TBS1996
changed the title
jjbayer
reviewed
Feb 16, 2024
| @@ -234,6 +234,15 @@ impl ProcessingGroup { | |||
| )) | |||
| } | |||
|
|
|||
| let is_metric = |i: &Item| matches!(i.ty(), ItemType::Statsd | ItemType::MetricBuckets); | |||
There was a problem hiding this comment.
Might be worth adding metrics meta to the integration test as well:
Suggested change
| let is_metric = |i: &Item| matches!(i.ty(), ItemType::Statsd | ItemType::MetricBuckets); | |
| let is_metric = |i: &Item| matches!(i.ty(), ItemType::Statsd | ItemType::MetricBuckets | ItemType::MetricMeta); |
We could add a is_metrics() method to ItemType and use it in queue_envelope and here.
tests/integration/test_metrics.py
Outdated
| project_id = 42 | ||
|
|
||
| timestamp = int(datetime.now(tz=timezone.utc).timestamp()) | ||
| metrics_payload = f"transactions/foo:42|c\ntransactions/bar:17|c|T{timestamp}" |
There was a problem hiding this comment.
Please also add a buckets payload and a metrics meta payload.
Dav1dde
reviewed
Feb 16, 2024
| project_key: envelope.meta().public_key(), | ||
| }) | ||
| // Remove metric meta from the envelope and send them directly to processing. | ||
| let metric_meta = envelope.take_items_by(|item| matches!(item.ty(), ItemType::MetricMeta)); |
There was a problem hiding this comment.
Nice! We didn't forget about metric meta.
jjbayer
approved these changes
Feb 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
should fix: #3042
Problem was we're not able to send metrics through without scoping from the projectstate, but in proxy mode the projectstate isn't available for non-static projects.
This PR simply skips the aggregation pipeline for metrics in proxy mode and send it to processing instead, which for metric groups does nothing and simply forwards it to upstream.