feat(metrics): Trace-connected Metrics (Implementation)

[Flash0ver]

Implementation of Trace-connected Metrics
See https://develop.sentry.dev/sdk/telemetry/metrics/

This changeset contains the initial implementation of Trace-connected Metrics for Sentry.

Changes

• made Batch-Buffer generic, so that it can be used for both Structured Logging and Trace-connected Metrics
• added new types (SentryTraceMetrics and derived), which follow the same pattern and structure from Structured Logs
• the SDK-APIs are generic, which are inspired by the System.Diagnostics.Metrics APIs of the BCL / System.Diagnostics.DiagnosticSource package
• naming of the public APIs differs a bit from the SDK spec in order to be idiomatic .NET
• currently, made APIs experimental

Please see the discussion comments below to discuss these design decisions.

Related changesets

• Tests: test(metrics): Trace-connected Metrics (Tests) #4839
• Samples: feat(metrics): Trace-connected Metrics (Samples) #4841
• Analyzers: feat(metrics): Trace-connected Metrics (Analyzers) #4840
• Documentation: docs(dotnet): Add metrics documentation for .NET sentry-docs#16054

Related issues and discussions

• Sentry Metrics for .NET #4741
• Metrics Docs #4749
• Metrics (Trace Connected) for .NET #4838

Follow-ups

• More default Attributes: feat(logs): apply default attributes from Scope #4784

Enable public beta

• Sentry Docs: dotnet platform
• Sentry Docs: Supported Platforms overview
• Sentry Onboarding
• then, enable in Sentry "Platform Categories"

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against c2108da

[codecov]

Codecov Report

❌ Patch coverage is 17.02703% with 307 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.59%. Comparing base (2370080) to head (c2108da).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry/SentryMetric.cs	0.00%	96 Missing and 2 partials ⚠️
src/Sentry/SentryMetric.Factory.cs	0.00%	41 Missing ⚠️
src/Sentry/SentryTraceMetricsCallbacks.cs	0.00%	36 Missing ⚠️
src/Sentry/Internal/DefaultSentryTraceMetrics.cs	26.31%	28 Missing ⚠️
src/Sentry/SentryUnits.cs	0.00%	24 Missing ⚠️
src/Sentry/Protocol/TraceMetric.cs	0.00%	14 Missing ⚠️
src/Sentry/SentryTraceMetrics.Distribution.cs	0.00%	10 Missing ⚠️
src/Sentry/SentryTraceMetrics.Gauge.cs	0.00%	10 Missing ⚠️
src/Sentry/SentryMetricType.cs	0.00%	9 Missing ⚠️
src/Sentry/SentryTraceMetrics.Counter.cs	0.00%	8 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4834      +/-   ##
==========================================
- Coverage   73.79%   72.59%   -1.21%     
==========================================
  Files         483      495      +12     
  Lines       17551    17893     +342     
  Branches     3461     3523      +62     
==========================================
+ Hits        12952    12989      +37     
- Misses       3746     4048     +302     
- Partials      853      856       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Flash0ver]

fixed in e65db5f

[cursor]

Missing null value check in SetAttributes methods

Medium Severity

The documentation states that null attribute values should be "ignored", and the public SetAttribute<T> method correctly returns early when value is null. However, the internal SetAttributes methods that accept IEnumerable<KeyValuePair<string, object>> and ReadOnlySpan<KeyValuePair<string, object>> don't check if attribute.Value is null before passing it to new SentryAttribute(attribute.Value). This inconsistency means users passing attribute dictionaries with null values get different behavior than documented, and could cause issues depending on how SentryAttribute handles null.

Additional Locations (1)

• src/Sentry/SentryMetric.cs#L259-L263

 

[github-actions]

• 🚫 The changelog entry seems to be part of an already released section ## 6.1.0.
  Consider moving the entry to the ## Unreleased section, please.

[Flash0ver]

question: discuss identifiers of public method groups

See spec in dev docs Metrics > Metrics Module

-Sentry.metrics.count(..)
+SentrySdk.Metrics.EmitCounter(..);
-Sentry.metrics.gauge(..)
+SentrySdk.Metrics.EmitGauge(..);
-Sentry.metrics.distribution(..)
+SentrySdk.Metrics.EmitDistribution(..);

• count is both a verb and a noun
• gauge is both a verb and a noun
• distribution is a noun only

So my thought was to - in particular for Distribution - use a verb as a prefix to form a verb phrase.
And then - for consistency - use a verb as a prefix for all three method groups.

The respective .NET Metrics (and Open Telemetry) are

using System.Diagnostics.Metrics;

static Meter meter = new Meter(name: "my-meter");

static Counter<int> counter = meter.CreateCounter<int>(name: "my-counter");
static Gauge<int> gauge = meter.CreateGauge<int>(name: "my-gauge");
static Histogram<int> histogram = meter.CreateHistogram<int>(name: "my-histogram");

counter.Add(delta: 1);
gauge.Record(value: 2);
histogram.Record(value: 3);

The first draft started with

SentrySdk.Metrics.AddCounter
SentrySdk.Metrics.RecordGauge
SentrySdk.Metrics.RecordDistribution

although with the verbs consistent with .NET/Open-Telemtry, these verbs do not fit, as they may suggest client-side aggregation, rather than sending the metric as-provided to Sentry with query-time aggregation at Sentry.

Then changing to the verb Emit. This verb is also used in the dev docs. The verb Emit is also used in Logs in Sentry for Go.

So the current naming scheme is, almost, Emit{Type}.
However, currently, instead of EmitCount, I decided to use EmitCounter instead, to be closer to the .NET BLC / System.Diagnostics.DiagnosticSource NuGet package.

An argument can be made to instead be consistent with the spec ... or at least more consistent with the spec. So here are some alternatives up for discussion.

// "type": "counter"
SentrySdk.Metrics.Count() // as in spec: "Sentry.metrics.count()"
SentrySdk.Metrics.EmitCount() // as verb phrase
SentrySdk.Metrics.EmitCounter() // Counter instead of Count, for consistency with the actual type name

// "type": "gauge"
SentrySdk.Metrics.Gauge() // as in spec: "Sentry.metrics.gauge()"
SentrySdk.Metrics.EmitGauge() // as verb phrase

// "type": "distribution"
SentrySdk.Metrics.Distribution() // as in spec: "Sentry.metrics.distribution()"
SentrySdk.Metrics.EmitDistribution() // as verb phrase

What do you prefer?

UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?

[jamescrosswell]

I don't think we have to worry about the API shape of the previous metrics product we had. I think the naming as you have it is fine.

[Flash0ver]

question: Overloads and Parameters of Method Groups

The SDK spec Metrics > Metrics Module suggests these parameters for the three methods Sentry.metrics.count, Sentry.metrics.gauge, and Sentry.metrics.distribution:

• name, String, required
• value, Number, required
• options, Object, optional
  • unit, String, optional, only for distribution and gauge, should not be an enum
  • attributes, Object, optional
  • scope, Scope, optional

name

I decided to have several overloads per method group, rather than a single method per type. This is also what the Meter type / Instrument types of the net6.0+ BCL APIs do.

value

With .NET being typed, I decided to make the APIs generic, as this is also what the .NET BCL / Open Telemetry / System.Diagnostics.DiagnosticSource NuGet package does.
This supports the numeric types byte, short, int, long, float, double. But don't support decimal (.NET Instruments do support it) as it's 128-bit, and Sentry only supports 64-bit signed integral types and 64-bit floating-point numbers.

options

Instead of an Options-Bag, I went with a method group and respective overloads, as again, they are similar to the System.Diagnostics.Metrics APIs.
However, if we'd like, we could do a struct-based Options-bag, per type (struct CounterOptions, struct GaugeOptions, and struct DistributionOptions).

unit

The unit overload only exists in the Gauge and Distribution method groups.
Typed to string, rather than Sentry.MeasurementUnit, as the SDK spec suggests to:

SDKs must not restrict what can be sent in (e.g. by using an enum) but should offer constants or similar that help customers send in units we support.

We could, however, offer an overload that takes Sentry.MeasurementUnit instead of a string.
Or we could add this overload later on.
The motivation of having a string rather than an enum is that Sentry may start supporting more Units at some time, and SDK users should be able to pass them once supported without upgrading the SDK.
Currently, a "custom" unit is not supported, and Relay will either discard or set the Unit to None if an unsupported unit is passed by the user --- the metric itself will not be discarded in that case.

attributes

Offer both an IEnumerable<KeyValuePair<string, object>>, and a ReadOnlySpan<KeyValuePair<string, object>> overload, again, very similar to "tags" of System.Diagnostics.Metrics.

scope

Just a nullable Scope.
Currently not available, but we could now - or later - add an overload for passing a Scope without a unit:
..(string name, T value, Scope? scope)

---

What do you think?

UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?

[jamescrosswell]

Instead of an Options-Bag, I went with a method group and respective overloads, as again, they are similar to the System.Diagnostics.Metrics APIs.

Using an Options type would mean less overloads to maintain... and if the options type was sealed then we could add members to it later. It's probably easier and less ceremony for SDK users to use various overloads with the options they care about.

It really depends how many dimensions the options type exposes... eventually (like with the SentryOptions) the only practical way to set these is via some kind of callback/delegate.

I think it's fine as is for now.

[jamescrosswell]

Currently not available, but we could now - or later - add an overload for passing a Scope without a unit:
..(string name, T value, Scope? scope)

What was the rational for omitting this overload currently?

[Flash0ver]

question: Design

With the Metric type being generic, supporting byte, short, int, long, float, and double, this API is also quite similar to the MeterListener of System.Diagnostics.Metrics.
Where you can set one callback per metric type via SetMeasurementEventCallback.

See Collect metrics.

What do you think?

UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?

[jamescrosswell]

It's a pain since the SDK user may want a callback like:

  if (metric.attributes.contains(a => a.name == "foo"))
  {
    return null;
  }

But they'd need to define that function once for each numeric type our metrics support and if we add support for new numeric types their code might break. It doesn't feel like a great API design.

An alternative option (not saying it's better, but just to consider) would be to have the parameter to the callback be a flattening of the various generic types... e.g.

SentryMetricArgType
{
    SentryMetricType type;
    string name;
    object value;
    string? unit;
    IEnumerable<KeyValuePair<string, object>> attributes;
    Scope? scope;
}

That might be a bit messy: impedance mismatch, mappings, type safety issues etc. but it might be better than the alternative.

A more complex variant would be to pass something like a SentryMetricTypeAccessor that mitigated some of the type safety issues by exposing properties of the metric that the SDK user might want to read via getter methods and exposed properties the SDK user might want to modify (if any) via setter methods... so they could do something like:

options.Metrics.SetBeforeSend(accessor => {
  if (accessor.GetMetricValue() is double && accessor.Attributes.Contains("foo"))
  {
   return null;
  }
});

Probably have to play around with different options to see how the implementation and SDK usage feels in practice. I could see what we have in this PR at the moment being pretty onerous for SDK users though, so I think we need to find a better option.

What solution have the other strongly typed SDKs (like Java) opted for here?

[Flash0ver]

question: Experimental vs Stable.

Do we want to publish it as experimental first ... or stable directly?

[jamescrosswell]

There's quite a lot of code so we could publish it as experimental for one point release... try to get some feedback from SDK customers using it in anger. There may be strong opinions some of the design decisions (e.g. method groups vs. options types/callbacks) so good to have the flexibility to revisit some of those decisions for a wee while if need be.

Otherwise it's more of a commercial question (is Sentry sure we're going to GA this metrics product?). I assume we captured enough learnings from the first metrics product to be pretty confident of the design and cost structure this time around. @dingsdax ?

[jamescrosswell]

We probably want to tidy up all of these change log entries when we include metrics in a non-preview release... For people who haven't been using the pre-releases we could just say:

- Add _experimental_ support for [Sentry trace-connected Metrics](https://docs.sentry.io/product/explore/metrics/) ([#4834](https://github.com/getsentry/sentry-dotnet/pull/4834))

[jamescrosswell]

Revert before merging into the main branch... to whatever the version in the main branch is. This gets set by the release process automatically.

Suggested change

	<VersionPrefix>6.1.0-alpha.2</VersionPrefix>
	<VersionPrefix>6.0.0</VersionPrefix>

[jamescrosswell]

Do we need a specific warning for trace connected metrics? In the past we've simply had SENTRY0001 to ensure SDK users deliberately opt into experimental features (like this one) and don't use these by accident in production.

[jamescrosswell]

Where does this get used?