Metrics Support - alpha #2949

jamescrosswell · 2023-12-06T03:34:49Z

Resolves #2880

Dogfooding

Once we have a nuget package, we can use the Symbol Collector to dogfood this.

src/Sentry/DelegatingMetricAggregator.cs

src/Sentry/DisabledMetricAggregator.cs

src/Sentry/MetricAggregator.cs

src/Sentry/IMetricAggregator.cs

src/Sentry/MetricAggregator.cs

src/Sentry/Protocol/Envelopes/Envelope.cs

src/Sentry/Protocol/Metrics/Metric.cs

src/Sentry/MetricAggregator.cs

bruno-garcia

Quick second pass

src/Sentry/MetricAggregator.cs

src/Sentry/Protocol/Metrics/Metric.cs

src/Sentry/Internal/Hub.cs

src/Sentry/MetricAggregator.cs

…rics

bruno-garcia · 2023-12-20T14:01:25Z

@jamescrosswell renamed the PR because we dropped the DDM internal name in favor of "Metrics"

bruno-garcia

Ranout of gas, didn't review MetricAggregator really and only did around 30/52 files.

CHANGELOG.md

bruno-garcia · 2023-12-20T14:05:55Z

modules/Ben.Demystifier

For reference: getsentry/Ben.Demystifier#4

samples/Sentry.Samples.Console.Metrics/Sentry.Samples.Console.Metrics.csproj

samples/Sentry.Samples.Console.Metrics/Program.cs

src/Sentry/Protocol/Metrics/DistributionMetric.cs

src/Sentry/Protocol/Envelopes/Envelope.cs

src/Sentry/MetricHelper.cs

src/Sentry/MetricAggregator.cs

bruno-garcia

Only got as far as 30/52 files and didn't go deep on MetricAggregator and other major pieces but sending some bits as it's what I managed to cover

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

…rics

jamescrosswell · 2023-12-20T22:50:12Z

Only got as far as 30/52 files and didn't go deep on MetricAggregator and other major pieces but sending some bits as it's what I managed to cover

Awesome - thank you! The meat of it is all in the MetricAggregator so that one might take a little while.

bruno-garcia

Now at 48/52 (only what really matters is left lol)

Maybe on a follow up PR we should rename MeasurementUnit, it's quite verbose

src/Sentry/MetricHelper.cs

src/Sentry/Protocol/Envelopes/Envelope.cs

src/Sentry/Protocol/Metrics/DistributionMetric.cs

src/Sentry/Protocol/Metrics/Metric.cs

src/Sentry/Timing.cs

test/Sentry.Tests/MetricAggregatorTests.cs

bruno-garcia · 2023-12-22T00:42:54Z

src/Sentry/MetricAggregator.cs

+
+    internal SentryStackFrame? GetCodeLocation(int stackLevel)
+    {
+        var stackTrace = new StackTrace(true);


this is a rather expensive API so i wonder if we can leverage https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.callermembernameattribute?view=net-8.0 instead

I don't see an easy way to do so. There's no way to supply a stackLevel to the CallerMemberName, that I can see. That's the main reason for the _seenLocations cache though... so at least this expensive operation only occurs once per unique metric per day.

bruno-garcia · 2023-12-22T00:44:18Z

src/Sentry/MetricAggregator.cs

+        _codeLocationLock.Wait();
+        try
+        {
+            var result = _pendingLocations;


You can do this atomically with Interlocked afaik, CompareExchange iirc

Hm, the _codeLocationLock is used elsewhere in the MetricAggregator to lock on different operations against _pendingLocations though. For example, there's this:

sentry-dotnet/src/Sentry/MetricAggregator.cs

Lines 210 to 214 in 1c3b275

if (!_pendingLocations.ContainsKey(startOfDay))

{

_pendingLocations[startOfDay] = new Dictionary<MetricResourceIdentifier, SentryStackFrame>();

}

_pendingLocations[startOfDay][metaKey] = location;

bruno-garcia

There's a race condition in RecordCodeLocation

src/Sentry/Protocol/Metrics/CodeLocations.cs

src/Sentry/MetricHelper.cs

src/Sentry/DelegatingMetricAggregator.cs

src/Sentry/MetricAggregator.cs

bitsandfoxes

This looks amazing!

src/Sentry/Timing.cs

src/Sentry/IMetricAggregator.cs

src/Sentry/MetricAggregator.cs

src/Sentry/Internal/Hub.cs

src/Sentry/Protocol/Metrics/CodeLocations.cs

src/Sentry/IMetricAggregator.cs

src/Sentry/Protocol/Envelopes/Envelope.cs

src/Sentry/SentryClient.cs

bruno-garcia

There are a few comments that I might have dropped outside a review that are pending. Could you please take a look on the files view: https://github.com/getsentry/sentry-dotnet/pull/2949/files

I don't see any blockers so I'm happy to merge this and ship a beta so we can test. It's just too hard to review this change now and we can do improvements/fixes in follow up PRs now.

src/Sentry/Timing.cs

bruno-garcia · 2024-01-05T18:47:25Z

src/Sentry/SentryClient.cs

+    internal void CaptureCodeLocations(CodeLocations codeLocations)
+    {
+        _options.LogDebug($"Capturing code locations for period: {codeLocations.Timestamp}");
+        CaptureEnvelope(Envelope.FromCodeLocations(codeLocations));


Are code locations not coupled with metrics? In which case they should be on the same envelope ideally

I think I did that because of this:
https://github.com/getsentry/sentry-python/blob/67c963d9c8d5e7e9de6347aee0edcf0c58d9fb24/sentry_sdk/metrics.py/#L580-L581

jamescrosswell added 8 commits December 5, 2023 12:09

Added basic metric types

5794a6c

Moved Metric classes

3752fd5

Added Increment aggregator

0d6587b

Implemented Gauge metric

088e4fd

Implemented Distribution and Set aggregations

9fac649

Added Metrics to ISentryClient API

c65355d

Update Hub.cs

b77c04d

Verify tests

c0233db

jamescrosswell requested review from bruno-garcia and vaind December 6, 2023 10:15

jamescrosswell added 2 commits December 7, 2023 11:10

Basic flush loop (no tests yet)

3b74b80

Implemented statsd serialization

c83e9c9

jamescrosswell changed the title ~~WIP: Delightful Developer Metrics pilot~~ WIP: Delightful Developer Metrics alpha Dec 7, 2023

jamescrosswell changed the title ~~WIP: Delightful Developer Metrics alpha~~ Delightful Developer Metrics alpha Dec 7, 2023

jamescrosswell requested a review from Giorgi December 7, 2023 09:20

jamescrosswell added 3 commits December 7, 2023 22:24

Update CHANGELOG.md

9150098

Split tests for Aggregagtor and BucketHelper

19dc74a

Update MetricBucketHelperTests.cs

cac0d79

mitsuhiko reviewed Dec 7, 2023

View reviewed changes

src/Sentry/DelegatingMetricAggregator.cs Outdated Show resolved Hide resolved

src/Sentry/DisabledMetricAggregator.cs Outdated Show resolved Hide resolved

Giorgi reviewed Dec 8, 2023

View reviewed changes

src/Sentry/MetricAggregator.cs Outdated Show resolved Hide resolved

bruno-garcia reviewed Dec 8, 2023

View reviewed changes

Integrated review feedback

721e6b3

bruno-garcia reviewed Dec 11, 2023

View reviewed changes

jamescrosswell added 2 commits December 11, 2023 18:09

Create MetricTests.verify.cs

a7ba47e

Updated verify tests

90be220

jamescrosswell mentioned this pull request Dec 11, 2023

The Hub should resolve the SentryClient in a consistent way #2962

Open

jamescrosswell added 4 commits December 11, 2023 20:15

Merge branch 'main' into metrics

73be790

Added Timing

94790d7

Merge branch 'metrics' of github.com:getsentry/sentry-dotnet into met…

e05f8ee

…rics

Added a (commented out) test to check if the aggregator is threadsafe

54af5c2

cleptric self-requested a review December 19, 2023 23:32

bruno-garcia changed the title ~~Delightful Developer Metrics alpha~~ Metrics Support - alpha Dec 20, 2023

bruno-garcia reviewed Dec 20, 2023

View reviewed changes

jamescrosswell and others added 5 commits December 21, 2023 10:03

Update CHANGELOG.md

7faa57c

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

Integrating review feedback

d956b5d

Source generated RegEx in metric helper

3efab8a

Merge branch 'metrics' of github.com:getsentry/sentry-dotnet into met…

62a7001

…rics

Update Envelope.cs

c68c25c

jamescrosswell self-assigned this Dec 20, 2023

Merge branch 'main' into metrics

1c3b275

bruno-garcia reviewed Dec 22, 2023

View reviewed changes

jamescrosswell added 2 commits December 27, 2023 21:47

Review feedback

4cb9ef7

Merge branch 'main' into metrics

36fe029

bruno-garcia requested changes Jan 2, 2024

View reviewed changes

jamescrosswell added 2 commits January 4, 2024 11:28

Integrating review feedback

8cc439c

More performant string delimited tags used in the bucket key

705d131

bitsandfoxes reviewed Jan 4, 2024

View reviewed changes

Integrating review feedback

353a928

bruno-garcia approved these changes Jan 5, 2024

View reviewed changes

jamescrosswell added 2 commits January 7, 2024 12:12

Integrated review feedback

589c3b2

Removed unused private field

bce6e50

bitsandfoxes approved these changes Jan 8, 2024

View reviewed changes

bruno-garcia merged commit 0fde2e3 into main Jan 8, 2024
19 checks passed

bruno-garcia deleted the metrics branch January 8, 2024 17:24

matejminar mentioned this pull request Jan 15, 2024

Add metrics onboarding docs for .NET getsentry/sentry#63180

Closed

jamescrosswell mentioned this pull request Jan 21, 2024

Improve test coverage for Metrics API #3066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics Support - alpha #2949

Metrics Support - alpha #2949

jamescrosswell commented Dec 6, 2023 •

edited

bruno-garcia left a comment

bruno-garcia commented Dec 20, 2023

bruno-garcia left a comment

bruno-garcia Dec 20, 2023

bruno-garcia left a comment

jamescrosswell commented Dec 20, 2023

bruno-garcia left a comment

bruno-garcia Dec 22, 2023

jamescrosswell Dec 27, 2023 •

edited

bruno-garcia Dec 22, 2023

jamescrosswell Dec 27, 2023

bruno-garcia left a comment

bitsandfoxes left a comment

bruno-garcia left a comment

bruno-garcia Jan 5, 2024

jamescrosswell Jan 6, 2024

	if (!_pendingLocations.ContainsKey(startOfDay))
	{
	_pendingLocations[startOfDay] = new Dictionary<MetricResourceIdentifier, SentryStackFrame>();
	}
	_pendingLocations[startOfDay][metaKey] = location;

Metrics Support - alpha #2949

Metrics Support - alpha #2949

Conversation

jamescrosswell commented Dec 6, 2023 • edited

Dogfooding

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia commented Dec 20, 2023

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia Dec 20, 2023

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

jamescrosswell commented Dec 20, 2023

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023

Choose a reason for hiding this comment

jamescrosswell Dec 27, 2023 • edited

Choose a reason for hiding this comment

bruno-garcia Dec 22, 2023

Choose a reason for hiding this comment

jamescrosswell Dec 27, 2023

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

bitsandfoxes left a comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia Jan 5, 2024

Choose a reason for hiding this comment

jamescrosswell Jan 6, 2024

Choose a reason for hiding this comment

jamescrosswell commented Dec 6, 2023 •

edited

jamescrosswell Dec 27, 2023 •

edited