feat(metrics): Rename all metrics extracted by relay [INGEST-548] #30472

jjbayer · 2021-12-07T14:38:21Z

As in getsentry/relay#1147, prefix all metric names with either sentry.sessions. or sentry.transactions..

jjbayer · 2021-12-07T14:42:09Z

@priscilawebdev or @matejminar could I please ask one of you to make the same changes on the front end? Basically just change the metric name for every API request in Performance from e.g. measurements.lcp to sentry.transactions.measurements.lcp. Feel free to push into this PR.

priscilawebdev · 2021-12-07T15:09:41Z

sure @jjbayer I'll do it 😉

src/sentry/relay/config.py

src/sentry/release_health/metrics.py

src/sentry/release_health/metrics_sessions_v2.py

ahmedetefy · 2021-12-07T17:54:08Z

src/sentry/snuba/metrics.py

@@ -470,28 +471,28 @@ class SnubaField:
 )

 _METRICS = {
-    "session": {
+    SessionMetricKey.SESSION.value: {


I would recommend here not adding the .value and just using the enum

I like the idea, but this dict also contains metric names that are not session metrics, so I think we should stick with strings here.

src/sentry/sentry_metrics/indexer/mock.py

github-actions · 2021-12-12T17:07:02Z

size-limit report

Path	Base Size (d7076ab)	Current Size	Change
src/sentry/static/sentry/dist/entrypoints/app.js	52.87 KB	52.87 KB	-0.01% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css	70.88 KB	70.88 KB	0%

ahmedetefy · 2021-12-13T09:56:18Z

src/sentry/release_health/metrics_sessions_v2.py

@@ -258,7 +257,7 @@ def get_values(
            started = int(data_points[key])
            abnormal = int(data_points.get(replace(key, raw_session_status="abnormal"), 0))
            crashed = int(data_points.get(replace(key, raw_session_status="crashed"), 0))
-            errored_key = replace(key, metric_name="session.error", raw_session_status=None)
+            errored_key = replace(key, metric_name=MetricKey.SESSION_ERROR, raw_session_status=None)


Should this be a .value ?

I changed the type of metric_name to MetricKey, but did not to rename the variable itself. Renamed to metric_key now to make it more clear.

jjbayer

@priscilawebdev Thank you for your contribution! I tested it locally and everything works as expected. Could you please review my latest commit? I accidentally added the wrong prefix to transaction.duration so I fixed that now, and renamed MeasurementMetric to TransactionMetric in the process.

priscilawebdev · 2021-12-13T11:04:08Z

static/app/utils/metrics/fields.tsx

+
+export const METRICS: Readonly<Record<SessionMetric | TransactionMetric, ColumnType>> = {
+  // Session metrics
+  [TransactionMetric.TRANSACTION_DURATION]: 'duration',


@jjbayer can you confirm that the types are correct here?

They look correct, the tricky thing is that count(transaction.duration) is actually an integer, while avg(transaction.duration), p95(transaction.duration) etc. are durations.

matejminar · 2021-12-13T11:08:28Z

tests/js/spec/views/performance/landing/widgets/widgetContainer.spec.jsx

@@ -710,7 +717,7 @@ describe('Performance > Widgets > WidgetContainer', function () {
        'p50 Duration'
      );

-      expect(wrapper.find('HighlightNumber').text()).toEqual('51s');
+      expect(wrapper.find('HighlightNumber').text()).toEqual('534ms');


Why is this assertion updated? I thought this is only renaming stuff, did some logic change?

yes this part of the code was updated

sentry/static/app/views/performance/landing/widgets/transforms/transformMetricsToArea.tsx

Line 87 in 6f110da

: mean(serie.data.map(({value}) => value));

matejminar · 2021-12-13T11:09:54Z

static/app/utils/metrics/fields.tsx

+  TRANSACTION_DURATION = 'sentry.transactions.transaction.duration',
+}
+
+export const METRICS: Readonly<Record<SessionMetric | TransactionMetric, ColumnType>> = {


Exported METRICS variable is very generic, can we please rename it to something more descriptive? Like METRIC_TO_COLUMN_TYPE.

matejminar · 2021-12-13T11:10:37Z

static/app/utils/metrics/fields.tsx

+
+export const METRICS: Readonly<Record<SessionMetric | TransactionMetric, ColumnType>> = {
+  // Session metrics
+  [TransactionMetric.TRANSACTION_DURATION]: 'duration',


Should this one be moved to the Transaction metrics group below?

matejminar · 2021-12-13T11:11:58Z

static/app/utils/metrics/fields.tsx

+  SESSION = 'sentry.sessions.session',
+  SESSION_DURATION = 'sentry.sessions.session.duration',
+  SESSION_ERROR = 'sentry.sessions.session.error',
+  USER = 'sentry.sessions.user',
+}
+
+export enum TransactionMetric {
+  MEASUREMENTS_FP = 'sentry.transactions.measurements.fp',
+  MEASUREMENTS_FCP = 'sentry.transactions.measurements.fcp',
+  MEASUREMENTS_LCP = 'sentry.transactions.measurements.lcp',
+  MEASUREMENTS_FID = 'sentry.transactions.measurements.fid',
+  MEASUREMENTS_CLS = 'sentry.transactions.measurements.cls',


nit: Would be nice to have prefixes sentry.sessions and sentry.transactions as a constant.

ahmedetefy · 2021-12-13T11:24:13Z

src/sentry/release_health/metrics_sessions_v2.py

@@ -542,7 +539,7 @@ def _flatten_data(org_id: int, data: _SnubaDataByMetric) -> _DataPoints:
            if raw_session_status is not None:
                raw_session_status = _reverse_resolve_ensured(raw_session_status)
            flat_key = _DataPointKey(
-                metric_name=metric_name,
+                metric_key=metric_name,


Maybe rename here the metric_name to metric_key then

feat: Rename all metrics extracted by relay (backend)

f9096d1

fix: Typo in tests

d294d52

vercel bot deployed to Preview – sentry December 7, 2021 14:45 View deployment

vercel bot deployed to Preview – storybook December 7, 2021 14:45 View deployment

jjbayer requested a review from ahmedetefy December 7, 2021 17:41

ahmedetefy reviewed Dec 7, 2021

View reviewed changes

src/sentry/relay/config.py Show resolved Hide resolved

ahmedetefy suggested changes Dec 7, 2021

View reviewed changes

Review feedback & test

e350e6b

vercel bot deployed to Preview – sentry December 9, 2021 10:41 View deployment

vercel bot deployed to Preview – storybook December 9, 2021 10:41 View deployment

Fix relay config test

9353ce1

vercel bot deployed to Preview – storybook December 9, 2021 11:51 View deployment

vercel bot deployed to Preview – sentry December 9, 2021 11:51 View deployment

priscilawebdev added 3 commits December 10, 2021 09:20

Merge branch 'master' into feat/rename-metrics

7f8b58a

update metrics name - frontend

e960db4

updated widget definitions

260c623

vercel bot deployed to Preview – sentry December 10, 2021 11:53 View deployment

vercel bot deployed to Preview – storybook December 10, 2021 11:53 View deployment

priscilawebdev added 2 commits December 12, 2021 16:45

Merge branch 'master' into feat/rename-metrics

2cdec0f

create metric fields

587f4b7

vercel bot deployed to Preview – storybook December 12, 2021 17:04 View deployment

vercel bot deployed to Preview – sentry December 12, 2021 17:04 View deployment

add session metrics to metrics

a9f7cae

vercel bot deployed to Preview – sentry December 12, 2021 17:07 View deployment

vercel bot deployed to Preview – storybook December 12, 2021 17:07 View deployment

ahmedetefy approved these changes Dec 13, 2021

View reviewed changes

ref: Rename _DataPointKey.metric_name to metric_key

f7617f1

jjbayer commented Dec 13, 2021

View reviewed changes

priscilawebdev reviewed Dec 13, 2021

View reviewed changes

matejminar reviewed Dec 13, 2021

View reviewed changes

fix frontend test

12b2c48

vercel bot deployed to Preview – sentry December 13, 2021 11:15 View deployment

vercel bot deployed to Preview – storybook December 13, 2021 11:15 View deployment

untitaker approved these changes Dec 13, 2021

View reviewed changes

cr feedback

7137d22

vercel bot deployed to Preview – sentry December 13, 2021 11:21 View deployment

vercel bot deployed to Preview – storybook December 13, 2021 11:21 View deployment

cr feedback

6f110da

vercel bot deployed to Preview – sentry December 13, 2021 11:23 View deployment

vercel bot deployed to Preview – storybook December 13, 2021 11:24 View deployment

ahmedetefy reviewed Dec 13, 2021

View reviewed changes

priscilawebdev approved these changes Dec 13, 2021

View reviewed changes

fix: Use metric_key everywhere

9487d16

vercel bot deployed to Preview – storybook December 13, 2021 14:35 View deployment

vercel bot deployed to Preview – sentry December 13, 2021 14:35 View deployment

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

c070a55

vercel bot deployed to Preview – storybook December 14, 2021 11:30 View deployment

vercel bot deployed to Preview – sentry December 14, 2021 11:30 View deployment

fix: Remove merge artifact

b2a21a4

vercel bot deployed to Preview – sentry December 14, 2021 11:47 View deployment

vercel bot deployed to Preview – storybook December 14, 2021 11:47 View deployment

fix tests

cf87df5

vercel bot deployed to Preview – sentry December 14, 2021 13:05 View deployment

vercel bot deployed to Preview – storybook December 14, 2021 13:05 View deployment

jjbayer merged commit 63fa2a1 into master Dec 14, 2021

jjbayer deleted the feat/rename-metrics branch December 14, 2021 13:45

github-actions bot locked and limited conversation to collaborators Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): Rename all metrics extracted by relay [INGEST-548] #30472

feat(metrics): Rename all metrics extracted by relay [INGEST-548] #30472

jjbayer commented Dec 7, 2021

jjbayer commented Dec 7, 2021

priscilawebdev commented Dec 7, 2021

ahmedetefy Dec 7, 2021

jjbayer Dec 9, 2021

github-actions bot commented Dec 12, 2021 •

edited

Loading

ahmedetefy Dec 13, 2021

jjbayer Dec 13, 2021

jjbayer left a comment

priscilawebdev Dec 13, 2021

jjbayer Dec 13, 2021

matejminar Dec 13, 2021

priscilawebdev Dec 13, 2021

matejminar Dec 13, 2021

matejminar Dec 13, 2021

matejminar Dec 13, 2021

ahmedetefy Dec 13, 2021

feat(metrics): Rename all metrics extracted by relay [INGEST-548] #30472

feat(metrics): Rename all metrics extracted by relay [INGEST-548] #30472

Conversation

jjbayer commented Dec 7, 2021

jjbayer commented Dec 7, 2021

priscilawebdev commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2021 • edited Loading

size-limit report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2021 •

edited

Loading