feat(metrics): Ignore transaction metrics allowlist [INGEST-1628] #1484

jjbayer · 2022-09-20T13:41:50Z

With custom measurements limited by the measurements config in event normalization (see #1483), we do not need the special allowlist for transaction metrics anymore.

Do not merge until getsentry/sentry#39001 has been deployed.

Note that after merging this PR, Sentry will still be sending the old config format to support old Relay instances. We will need a bump of transactionMetrics.VERSION to finally clean up that part.

#skip-changelog

…easurements

jjbayer · 2022-09-20T13:44:51Z

relay-server/src/metrics_extraction/transactions.rs

-            assert_eq!(metric.tags["platform"], "javascript");
-            assert!(!metric.tags.contains_key("bogus"));
-        }
+        insta::assert_debug_snapshot!(metrics, @r###"


The test diff is large because a few tests used the metrics allowlist to filter down metrics returned. In the process, I decided to replace ad-hoc assert statements with insta::assert_debug_snapshot! where possible.

The test diff is large because a few tests used the metrics allowlist to filter down metrics returned.

Definitely out of the scope of this PR, but tests should not interfere with other tests.

I like using snapshots (vs ad-hoc asserts) a lot. However, making this change in this PR makes focusing on the relevant aspects harder, vs having another PR for the refactor. No action required on this PR, just a note for future PRs.

Agreed. Splitting the PRs is actually not that hard, so I did it here: #1500.

jjbayer · 2022-09-20T13:45:28Z

relay-server/src/metrics_extraction/transactions.rs

        let config: TransactionMetricsConfig = serde_json::from_str(
            r#"
        {
+            "version": 1,
            "extractMetrics": [


TODO: remove extractMetrics from all test cases.

…-allowlist

iker-barriocanal

Not blocking, but I want to understand the removal of total.time first.

iker-barriocanal · 2022-09-26T08:16:52Z

relay-server/src/metrics_extraction/transactions.rs

+                if measurement_name == "total.time" {
+                    // The only reason we do not emit total.time as a metric is that is was not
+                    // on the allowlist in sentry before, and nobody seems to be missing it.
+                    continue;
+                }


I don't understand why we should remove it. If it's not in the allowlist and a user adds it, my expectation is either to treat is as a custom measurement or to just not support it (e.g. it could be a typo).

total.time is a special case because it is generated by Relay itself:

relay/relay-general/src/store/normalize/breakdowns.rs

Lines 144 to 148 in 2a63da2

let total_time_value = Annotated::new(Measurement {

value: Annotated::new(relay_common::duration_to_millis(total_time)),

unit: Annotated::new(MetricUnit::Duration(DurationUnit::MilliSecond)),

});

breakdown.insert("total.time".to_string(), total_time_value);

Even if a user sets it, Relay overwrites it.

It probably makes sense to extract total.time as a metric, but there are three reasons why I wouldn't do it in this PR:

It was previously not in the allowlist, and I don't want this PR to change anything about our externally visible behavior in production.

Since total.time is generated by Relay, we would now emit an additional metric for every transaction that has breakdowns, something that we should coordinate with storage.

(more practical): There is an integration test that will break Relay CI if we add a metric: https://github.com/getsentry/sentry/blob/72ed3980d2113aed3c44d7bcec18d1d7513c074f/tests/relay_integration/test_metrics_extraction.py#L14. This is fine if we want to deliberately add the metric, but I think that should be a separate PR.

cc @jan-auer

Sounds good, thanks for the explanation.

Out of the scope of the PR, but we should make a decision on if we want to keep it or not. If nobody misses it (or can be calculated from other metrics), we can remove it from the codebase instead of generating and discarding it.

iker-barriocanal · 2022-09-26T08:19:33Z

relay-server/src/metrics_extraction/transactions.rs

-            assert_eq!(metric.tags["platform"], "javascript");
-            assert!(!metric.tags.contains_key("bogus"));
-        }
+        insta::assert_debug_snapshot!(metrics, @r###"


The test diff is large because a few tests used the metrics allowlist to filter down metrics returned.

Definitely out of the scope of this PR, but tests should not interfere with other tests.

iker-barriocanal · 2022-09-26T08:23:10Z

relay-server/src/metrics_extraction/transactions.rs

-            assert_eq!(metric.tags["platform"], "javascript");
-            assert!(!metric.tags.contains_key("bogus"));
-        }
+        insta::assert_debug_snapshot!(metrics, @r###"


I like using snapshots (vs ad-hoc asserts) a lot. However, making this change in this PR makes focusing on the relevant aspects harder, vs having another PR for the refactor. No action required on this PR, just a note for future PRs.

tests/integration/test_metrics.py

Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

Replacing ad-hoc asserts in tests with `insta::assert_debug_snapshot!` should improve readability and make it easier to adapt the tests when functionality changes (see #1484).

…-allowlist

iker-barriocanal · 2022-09-27T08:23:19Z

relay-server/src/metrics_extraction/transactions.rs

+                if measurement_name == "total.time" {
+                    // The only reason we do not emit total.time as a metric is that is was not
+                    // on the allowlist in sentry before, and nobody seems to be missing it.
+                    continue;
+                }


Sounds good, thanks for the explanation.

Out of the scope of the PR, but we should make a decision on if we want to keep it or not. If nobody misses it (or can be calculated from other metrics), we can remove it from the codebase instead of generating and discarding it.

* master: feat(metrics): Ignore transaction metrics allowlist (#1484) ref(metrics): Remove long running futures (#1492) ref(metrics): Convert metrics tests to insta (#1500) doc(metrics): Add bucket width to examples (#1496)

This reverts commit be12cd4.

In #1484, we stopped using the old project config entries `transactionMetrics.extractMetrics` and `transactionMetrics.customMeasurements` because they have become unnecessary for Relay. However, they are needed by outdated downstream Relays to function correctly, so they should be part of the project configs we serialize for use in downstream instances.

jjbayer added 14 commits September 19, 2022 14:07

wip

62dce92

feat: Limit custom measurements

dc61d75

test: limit custom measurements

8800cce

fix

bef558d

Merge remote-tracking branch 'origin/master' into feat/limit-custom-m…

ceac3cd

…easurements

doc: Changelog

98539fe

test: Add integration test for custom measurements

d12b1ee

test: Add new measurements config to integration test

619f92e

feat: Ignore metrics allowlist

205d97a

ref: Remove push_metric and old config format

b4a3720

test: Fix grand transaction metrics test and make it insta

8be0663

test: fix

aca1dad

wip

0eb1886

fix: normalization test

6aee1f6

jjbayer self-assigned this Sep 20, 2022

jjbayer commented Sep 20, 2022

View reviewed changes

Base automatically changed from feat/limit-custom-measurements to master September 22, 2022 11:41

jjbayer and others added 11 commits September 22, 2022 14:32

Merge remote-tracking branch 'origin/master' into feat/ignore-metrics…

c85f51e

…-allowlist

fix: test

9552ee1

ref: Remove old config from tests

2d68a4a

fix(lint): make sure lint and test work for rust 1.64.0

192378d

fix(lint): remove reference

3cd51a9

Merge branch 'fix/lint-rust-1.64.0' into feat/ignore-metrics-allowlist

a1d2978

fix: more tests

bcaac5a

Merge remote-tracking branch 'origin/master' into feat/ignore-metrics…

1da7b9e

…-allowlist

ref: Revert distracting changes

ee7cc8e

test: update config format

938169c

fix: Do not emit total.time

5b37e1a

jjbayer marked this pull request as ready for review September 23, 2022 11:15

jjbayer requested a review from a team September 23, 2022 11:15

jjbayer assigned iker-barriocanal and unassigned jjbayer Sep 23, 2022

iker-barriocanal reviewed Sep 26, 2022

View reviewed changes

iker-barriocanal assigned jjbayer and unassigned iker-barriocanal Sep 26, 2022

Update tests/integration/test_metrics.py

2a63da2

Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

jjbayer mentioned this pull request Sep 26, 2022

ref(metrics): Convert metrics tests to insta [INGEST-1502] #1500

Merged

jjbayer requested a review from iker-barriocanal September 26, 2022 09:51

jjbayer assigned iker-barriocanal and unassigned jjbayer Sep 26, 2022

jjbayer added 2 commits September 26, 2022 14:13

Merge remote-tracking branch 'origin/master' into feat/ignore-metrics…

4445783

…-allowlist

ref: Change indent to reduce diff with master

4370e3e

iker-barriocanal approved these changes Sep 27, 2022

View reviewed changes

iker-barriocanal assigned jjbayer and unassigned iker-barriocanal Sep 27, 2022

jjbayer merged commit be12cd4 into master Sep 27, 2022

jjbayer deleted the feat/ignore-metrics-allowlist branch September 27, 2022 10:52

jjbayer added a commit that referenced this pull request Sep 27, 2022

Revert "feat(metrics): Ignore transaction metrics allowlist (#1484)"

5ce5102

This reverts commit be12cd4.

jjbayer mentioned this pull request Sep 27, 2022

fix(metrics): Restore allowlist in metrics config [INGEST-1502] #1502

Merged

jjbayer mentioned this pull request Feb 23, 2023

feat(dynamic-sampling): Implement prioritize by project bias [TET-574] getsentry/sentry#42939

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): Ignore transaction metrics allowlist [INGEST-1628] #1484

feat(metrics): Ignore transaction metrics allowlist [INGEST-1628] #1484

jjbayer commented Sep 20, 2022 •

edited

Loading

jjbayer Sep 20, 2022

iker-barriocanal Sep 26, 2022

iker-barriocanal Sep 26, 2022

jjbayer Sep 26, 2022

jjbayer Sep 20, 2022

iker-barriocanal left a comment

iker-barriocanal Sep 26, 2022

jjbayer Sep 26, 2022

iker-barriocanal Sep 27, 2022

iker-barriocanal Sep 26, 2022

iker-barriocanal Sep 26, 2022

iker-barriocanal Sep 27, 2022

	let total_time_value = Annotated::new(Measurement {
	value: Annotated::new(relay_common::duration_to_millis(total_time)),
	unit: Annotated::new(MetricUnit::Duration(DurationUnit::MilliSecond)),
	});
	breakdown.insert("total.time".to_string(), total_time_value);

feat(metrics): Ignore transaction metrics allowlist [INGEST-1628] #1484

feat(metrics): Ignore transaction metrics allowlist [INGEST-1628] #1484

Conversation

jjbayer commented Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iker-barriocanal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer commented Sep 20, 2022 •

edited

Loading