Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Custom Measurements API #2013

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Add Custom Measurements API #2013

merged 10 commits into from
Oct 27, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Oct 26, 2022

Adds support for Custom Performance Metrics
https://docs.sentry.io/product/performance/metrics/#custom-performance-metrics

Usage:

// integer values can be int, long, or ulong
transaction.SetMeasurement("some_time", 50, MeasurementUnit.Duration.Millisecond);

// floating point values are passed as double
transaction.SetMeasurement("file_size", 1.21, MeasurementUnit.Information.Gigabyte);

// set measurement without unit will omit sending a unit to Sentry
transaction.SetMeasurement("num_widgets", 10);

// you can also explicitly set "none" as the unit sent to Sentry
transaction.SetMeasurement("num_widgets", 10, MeasurementUnit.None);

// custom units can be used as well
transaction.SetMeasurement("foo", 5000, MeasurementUnit.Custom("bar"));

In Sentry, they will show up under Performance > Transaction Summary > Event Details like this:

image

Resolves #1951

@marandaneto
Copy link
Contributor

@mattjohnsonpint a small note:

// set measurement without unit is the same as passing MeasurementUnit.None
transaction.SetMeasurement("num_widgets", 10);

If no unit is given, it should not have a unit rather than defaulting to MeasurementUnit.None.
Relay will set them to "none" as a default.

One should be able to transaction.SetMeasurement("num_widgets", 10, MeasurementUnit.None);

@mattjohnsonpint
Copy link
Contributor Author

@marandaneto - Within the SDK, MeasuementUnit.None.ToString() == "", and "" will omit sending the unit field during json serialization. It won't ever send "unit":"none" or "unit":"" in the json either way.

@mattjohnsonpint
Copy link
Contributor Author

I added some more tests to demonstrate / ensure that. Thanks!

@marandaneto
Copy link
Contributor

@marandaneto - Within the SDK, MeasuementUnit.None.ToString() == "", and "" will omit sending the unit field during json serialization. It won't ever send "unit":"none" or "unit":"" in the json either way.

Actually none is a valid measurement, so if MeasuementUnit.None is given, the unit should be serialized to none.
If no unit is given, then you don't serialize anything.

@mattjohnsonpint
Copy link
Contributor Author

Interesting... I read it differently I guess. I will double check. Thanks for highlighting that! 👍

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Oct 26, 2022

Hmmm... I'm not so sure.

In https://getsentry.github.io/relay/relay_metrics/enum.MetricUnit.html#variant.None

None
Untyped value without a unit ("").

But in https://develop.sentry.dev/sdk/event-payloads/transaction/

Custom measurements need units to be specified; if the units are missing, Relay will set them to "none" as a default.

Both docs are describing Relay, so one of them must be in error. 🤔

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Oct 26, 2022

Looking at the Relay source code, it appears either "none" or "" are both treated as None.

https://github.com/getsentry/relay/blob/91250fc1b52e66418830e7b6051887dbfea8662a/relay-common/src/constants.rs#L639

Thus, I conclude all of these would be identical:

{"value":"123"}
{"value":"123","unit":""}
{"value":"123","unit":"none"}

(For the .NET SDK, I always use the first one.)

@marandaneto
Copy link
Contributor

Looking at the Relay source code, it appears either "none" or "" are both treated as None.

getsentry/relay@91250fc/relay-common/src/constants.rs#L639

Thus, I conclude all of these would be identical:

{"value":"123"}
{"value":"123","unit":""}
{"value":"123","unit":"none"}

(For the .NET SDK, I always use the first one.)

Yep, but that's what I got from the Ingest folks, Relay is a good citizen and accepts everything but the SDK is;

Prefer {"value":"123"} over {"value":"123","unit":"none"}, the later only if the user set MeasuementUnit.None.
{"value":"123","unit":""} isn't allowed by the SDKs, but it's accepted by Relay because of back-compatibility reasons most likely.
You can see that I've removed accepting empty on this PR https://github.com/getsentry/sentry-docs/pull/5598/files L54

@mattjohnsonpint
Copy link
Contributor Author

So I think the differences boil down to:

the later only if the user set MeasuementUnit.None

I still don't understand why it would matter though. Given two ways to serialize the data where one is 14 chars longer and they both have the same effect, I prefer the shorter form.

@marandaneto
Copy link
Contributor

marandaneto commented Oct 27, 2022

MeasuementUnit.None

MeasuementUnit.None is a valid unit and it should be sent as such.
Omitting the unit defaults to none (on Relay), yes, but maybe tomorrow it'd be something else and this would be a breaking change for the former use case if doing what you've suggested.
The other reason is to align with all the other SDKs.
There was a DACI for this, so I'd not spend too much though on it.

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Oct 27, 2022

@marandaneto - I updated as you advised, to align with the behavior of the other SDKs.

  • transaction.SetMeasurement("foo", 123) will send "foo":{"value":"123"}
  • transaction.SetMeasurement("foo", 123, MeasurementUnit.None) will send "foo":{"value":"123","unit":"none"}

Thanks for your help! 😃

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@mattjohnsonpint mattjohnsonpint merged commit 3b73dd3 into main Oct 27, 2022
@mattjohnsonpint mattjohnsonpint deleted the custom-measurements branch October 27, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set custom measurements on transactions
3 participants