feat(relay): Move histogram outliers to global config #71004
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
github-actions
bot
added
the
Scope: Backend
jjbayer
commented
May 17, 2024
| @@ -1427,37 +1433,48 @@ def _produce_histogram_outliers(query_results: Any) -> Sequence[MetricConditiona | |||
| # See also https://en.wikipedia.org/wiki/Outlier#Tukey's_fences | |||
| { | |||
| "op": "gte", | |||
| "name": "event.duration", | |||
| "name": _HISTOGRAM_OUTLIERS_SOURCE_FIELDS[metric], | |||
There was a problem hiding this comment.
This fixes a bug that went unnoticed for a long time: p25 and p75 are either duration, LCP, or FCP, but the value we compare them to is always duration.
| @@ -1013,7 +1013,7 @@ def _filter_option_to_config_setting(flt: _FilterSpec, setting: str) -> Mapping[ | |||
| #: When you increment this version, outdated Relays will stop extracting | |||
| #: transaction metrics. | |||
| #: See https://github.com/getsentry/relay/blob/6181c6e80b9485ed394c40bc860586ae934704e2/relay-dynamic-config/src/metrics.rs#L85 | |||
| TRANSACTION_METRICS_EXTRACTION_VERSION = 5 | |||
| TRANSACTION_METRICS_EXTRACTION_VERSION = 6 | |||
There was a problem hiding this comment.
Version bump needed because old Relays do not apply globally defined tags to legacy metrics (see getsentry/relay#3615).
| pass | ||
|
|
||
| consumer.close() | ||
| assert histogram_outlier_tags == { |
There was a problem hiding this comment.
This assertion will fail until getsentry/relay#3615 is merged (requires up to date relay docker image).
Dav1dde
approved these changes
May 17, 2024
jjbayer
added a commit
to getsentry/relay
that referenced
this pull request
May 21, 2024
Prerequisite to getsentry/sentry#71004: Global tag mappings were not yet being applied to transaction metrics. Beside the bug fix, this PR also bumps the metrics extraction version(s), and attempts to simplify the extraction function for transactions.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #71004 +/- ##
=======================================
Coverage 77.85% 77.86%
=======================================
Files 6533 6532 -1
Lines 291110 291110
Branches 50377 50372 -5
=======================================
+ Hits 226657 226660 +3
+ Misses 58218 58211 -7
- Partials 6235 6239 +4
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Histogram outliers are a static piece of metrics extraction config that can be moved to global config. This will reduce network traffic, project config sizes in redis and first and foremost relay in-memory caches.
This PR also fixes a long-standing bug in the outlier config.
ref: https://github.com/getsentry/team-ingest/issues/331