feat(measurements): Add new measurements section to transaction #515

sl0thentr0py · 2022-03-03T17:36:47Z

No description provided.

vercel · 2022-03-03T17:36:50Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/develop/8uGACV9ZwrynqT9fe7eLUyHFxPL4
✅ Preview: https://develop-git-neel-webbackend-109-measurements.sentry.dev

sl0thentr0py · 2022-03-03T17:39:39Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+
+: _Optional_. An object containing standard/custom measurements with keys signifying the name of the measurement.
+
+Standard measurement keys currently supported are from the set  `"fp" | "lcp" | "fid" | "ttfb" | "ttfb.requesttime"`.


I took these from here and also from the various setters in sentry-javascript/metrics. Feel free to add/update this list.

Hmm interesting that the mobile vitals are not in the Sentry allow list. So you can't do discover arithmetic on them?

there is another list here which also has cls.

Hmm perhaps @wmak can help figure this out here

Hmm not being able to do arithmetic on vitals is definitely an oversight on my part (ie. we didn't update this allowlist when working on it)

But i believe the standard measurement list should be everything we prefix measurements on in the UI? ie. we should include the ones here that aren't rates or percentages (those are computed)

sl0thentr0py · 2022-03-03T17:40:58Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+
+Standard measurement keys currently supported are from the set  `"fp" | "lcp" | "fid" | "ttfb" | "ttfb.requesttime"`.
+
+Custom measurements need units to be specified. Units currently supported are durations from the set  `"ns" | "ms" | "s"`.


and these I took from relay/MetricUnit.

Could you check which units would actually make sense and how we would like to specify units? What we have in Relay was only a stub, and now is a good time to rethink this. Generally, we have two options:

Define a strict allow list of units that we define in Relay, and then propagate down.

Allow arbitrary strings as unit. Relay still lists well-known units that it can handle, eg. for conversion.

ok so making a list of potentially useful categories, maybe product people have to add here.

type units remarks

time ns, microseconds, ms, s, m longer than minute not useful, formatting for microseconds

memory bytes, KiB, MiB, GiB, TiB KB vs KiB

unitless / absolute counts

unitless / percentages %

Formatting consistently for some cases is problematic if we go with short forms, for instance microseconds vs ns and I don't know if we want to get into unicode mess here.

I'm not sure of the value provided by arbitrary user strings.
There's no semantic associated with a random foobar unit, so I assume it will just be a passthrough all the way to wherever the metric will be displayed on the UI. I think it is more valuable to actually look at valid measurement use cases and we can always extend our supported list with things that make sense.

ok looking around a bit, should have done this first

Datadog

The Unified Code for Units of Measure

OpenTelemetry unmerged PR on units spec

OpenTelemetry current guidelines

AbhiPrasad

Pinging Kevan to also take a look here.

AbhiPrasad · 2022-03-03T17:39:37Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+
+: _Optional_. An object containing standard/custom measurements with keys signifying the name of the measurement.
+
+Standard measurement keys currently supported are from the set  `"fp" | "lcp" | "fid" | "ttfb" | "ttfb.requesttime"`.


Can we clarify what "supported" means here?

Also, what do you think about linking to the relay/sentry allow list code here?

This is all browser web vitals, we also need to include the mobile vitals here as well.

I think 'supported' is open to discussion based on either

what is currently implemented end-end in browser/mobile

what gets assigned units automatically if we say we need units for custom measurements from the user (?)

marandaneto · 2022-03-04T13:53:18Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+  "frames_slow_rate",
+  "frames_frozen_rate",


I believe we don't use them, @philipphofmann what's about on iOS?

No, we don't use frames_slow_rate and frames_frozen_rate, but we use the others above.

marandaneto · 2022-03-04T13:53:33Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+  "stall_count",
+  "stall_total_time",
+  "stall_longest_time",
+  "stall_percentage"


I didn't find the usage of this one on RN.

It's referenced by Sentry: https://github.com/getsentry/sentry/blob/7447331d8e02031d585bd0c0aec76ed1334b9bde/src/sentry/search/events/constants.py#L32

Couldn't find it in Relay, not sure how this works. Maybe @untitaker can help.

One of the open questions here is if relay will 'know' the difference between standard/custom measurements or if this will only be reflected in the sentry codebase.

Regarding stall_* measurements: I searched through all of getsentry, and it looks like they were only defined in Sentry. Later on, we pulled them into Relay and the ingest load tester. If they are not emitted by any SDK, we should remove them.

Regarding units: Relay will have to know about units of the past built-in measurements for backwards compatibility. For new measurements, and also going forward for all SDKs, it would be better to declare all units on the client and send them in to avoid mismatches and outdated lists.

~~if I understood @jan-auer correctly, relay will have to know existing/standard measurements to infer their units.~~

~~currently only sentry has such a list, whether that list moves to Relay is tbd (we currently send it from sentry to relay via projectconfig)~~

EDIT: jinx

Thanks, and the rates / percentages are defined in Sentry so they don't have to be added to the public interface:
https://github.com/getsentry/sentry/blob/1c4ee6be215dadbf9bec19b3b428464a1dfe05b3/src/sentry/search/events/fields.py#L380-L418

Actually, I'm wondering whether we should mark them as reserved and then never use them. They'll be defined as derived metrics in Sentry and can never be ingested.

Let's not do any special casing for the three computed "measurements", but document them as reserved. That means, we should not reject the names in SDKs or during ingestion, but we should let people know that the name is used in Sentry.

There can be multiple metrics with the same name, and they are disambiguated in the API by type, so in the product we'll be fine. I'm still working out what that would mean for Discover though.

@wmak sharing a thought here that we can follow up on somewhere else: We should evaluate renaming those three specific measurements:

Two are called *_rate like everything else in the product, but one is called _percentage. As opposed to functions like failure_rate() they can be computed within a single transaction though, if I'm not mistaken? So the question is do they behave like measurements (i.e. distributions), then we should materialize them, if they are aggregate percentages, they should rather become functions.

They share the same namespace with ingested measurements but they are not actually ingested. I wonder if they should actually be materialized in Relay.

They can be computed within a single transaction 👍
Is it possible to materialize these in relay if they're going to be a metric as well? Would simplify the product a bit

Yes, in that case we can do the division in Relay, write them back into the measurements array, before extracting metrics for all measurements including the computed ones. I'd have to think a bit about migrating this forward, for a while we should probably keep the computation in discover around until we've reached 90d of history.

bruno-garcia

Are we including measurements that are not sent by the client but are generated on the backend?

bruno-garcia · 2022-03-04T16:18:00Z

src/docs/sdk/event-payloads/properties/measurements.mdx

+  "stall_count",
+  "stall_total_time",
+  "stall_longest_time",
+  "stall_percentage"


It's referenced by Sentry: https://github.com/getsentry/sentry/blob/7447331d8e02031d585bd0c0aec76ed1334b9bde/src/sentry/search/events/constants.py#L32

Couldn't find it in Relay, not sure how this works. Maybe @untitaker can help.

…-measurements

sl0thentr0py · 2022-03-09T12:51:46Z

Summarizing the discussion so far.

I've removed frames_slow_rate, frames_frozen_rate and stall_percentage since they are computed/derived from existing meaurements (ref: jira).
The units topic is a larger discussion that will be continued in a separate PR but for now:

We will not go with arbitrary user defined unit strings but try to identify use cases which make sense.
- The downside to this is SDKs will need validation on the unit so users don't send random stuff. If someone has opinions on this, let us know.
Currently we will just go with time/duration units [ns, ms, s] that are already defined in MetricUnit in relay.

feat(measurements): Add new measurements section to transaction

550d98c

sl0thentr0py requested review from AbhiPrasad and jan-auer March 3, 2022 17:36

sl0thentr0py commented Mar 3, 2022

View reviewed changes

AbhiPrasad requested a review from k-fish March 3, 2022 17:41

AbhiPrasad reviewed Mar 3, 2022

View reviewed changes

Add fcp and cls

2633b10

vercel bot deployed to Preview March 3, 2022 17:52 View deployment

Add mobile keys too; add github links

9986098

vercel bot deployed to Preview March 3, 2022 18:10 View deployment

List instead of set

1b195d5

vercel bot deployed to Preview March 3, 2022 18:11 View deployment

philipphofmann requested review from philipphofmann, bruno-garcia and marandaneto March 4, 2022 13:50

marandaneto reviewed Mar 4, 2022

View reviewed changes

sl0thentr0py mentioned this pull request Mar 4, 2022

feat(measurements): Add experimental set_measurement api on transaction getsentry/sentry-python#1359

Merged

bruno-garcia reviewed Mar 4, 2022

View reviewed changes

bruno-garcia requested a review from mattjohnsonpint March 4, 2022 20:18

Merge remote-tracking branch 'origin/master' into neel/WEBBACKEND-109…

3ae5c4f

…-measurements

vercel bot deployed to Preview March 9, 2022 12:23 View deployment

Remove computed metrics and add future note about units

8fe0a2d

vercel bot deployed to Preview March 9, 2022 12:40 View deployment

sl0thentr0py merged commit 220dbf3 into master Mar 9, 2022

sl0thentr0py deleted the neel/WEBBACKEND-109-measurements branch March 9, 2022 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(measurements): Add new measurements section to transaction #515

feat(measurements): Add new measurements section to transaction #515

sl0thentr0py commented Mar 3, 2022

vercel bot commented Mar 3, 2022 •

edited

Loading

sl0thentr0py Mar 3, 2022

AbhiPrasad Mar 3, 2022

sl0thentr0py Mar 3, 2022

AbhiPrasad Mar 3, 2022

wmak Mar 3, 2022

sl0thentr0py Mar 3, 2022

jan-auer Mar 8, 2022

sl0thentr0py Mar 8, 2022

sl0thentr0py Mar 8, 2022

sl0thentr0py Mar 8, 2022 •

edited

Loading

AbhiPrasad left a comment

AbhiPrasad Mar 3, 2022

sl0thentr0py Mar 3, 2022

marandaneto Mar 4, 2022

philipphofmann Mar 7, 2022

marandaneto Mar 4, 2022

bruno-garcia Mar 4, 2022

sl0thentr0py Mar 4, 2022 •

edited

Loading

jan-auer Mar 7, 2022

untitaker Mar 7, 2022 •

edited

Loading

jan-auer Mar 7, 2022

jan-auer Mar 7, 2022

jan-auer Mar 8, 2022 •

edited

Loading

wmak Mar 8, 2022

jan-auer Mar 9, 2022

bruno-garcia left a comment

bruno-garcia Mar 4, 2022

sl0thentr0py commented Mar 9, 2022


		: _Optional_. An object containing standard/custom measurements with keys signifying the name of the measurement.

		Standard measurement keys currently supported are from the set `"fp" \| "lcp" \| "fid" \| "ttfb" \| "ttfb.requesttime"`.


		Standard measurement keys currently supported are from the set `"fp" \| "lcp" \| "fid" \| "ttfb" \| "ttfb.requesttime"`.

		Custom measurements need units to be specified. Units currently supported are durations from the set `"ns" \| "ms" \| "s"`.

type	units	remarks
time	ns, microseconds, ms, s, m	longer than minute not useful, formatting for microseconds
memory	bytes, KiB, MiB, GiB, TiB	KB vs KiB
unitless / absolute counts
unitless / percentages	%

feat(measurements): Add new measurements section to transaction #515

feat(measurements): Add new measurements section to transaction #515

Conversation

sl0thentr0py commented Mar 3, 2022

vercel bot commented Mar 3, 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

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

sl0thentr0py Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

AbhiPrasad 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

sl0thentr0py Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sl0thentr0py commented Mar 9, 2022

vercel bot commented Mar 3, 2022 •

edited

Loading

sl0thentr0py Mar 8, 2022 •

edited

Loading

sl0thentr0py Mar 4, 2022 •

edited

Loading

untitaker Mar 7, 2022 •

edited

Loading

jan-auer Mar 8, 2022 •

edited

Loading