Conversation
|
@jamescrosswell renamed the PR because we dropped the |
bruno-garcia
left a comment
There was a problem hiding this comment.
Ranout of gas, didn't review MetricAggregator really and only did around 30/52 files.
samples/Sentry.Samples.Console.Metrics/Sentry.Samples.Console.Metrics.csproj
Outdated
Show resolved
Hide resolved
samples/Sentry.Samples.Console.Metrics/Sentry.Samples.Console.Metrics.csproj
Outdated
Show resolved
Hide resolved
bruno-garcia
left a comment
There was a problem hiding this comment.
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>
Awesome - thank you! The meat of it is all in the MetricAggregator so that one might take a little while. |
bruno-garcia
left a comment
There was a problem hiding this comment.
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
|
|
||
| internal SentryStackFrame? GetCodeLocation(int stackLevel) | ||
| { | ||
| var stackTrace = new StackTrace(true); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| _codeLocationLock.Wait(); | ||
| try | ||
| { | ||
| var result = _pendingLocations; |
There was a problem hiding this comment.
You can do this atomically with Interlocked afaik, CompareExchange iirc
There was a problem hiding this comment.
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
bruno-garcia
left a comment
There was a problem hiding this comment.
There's a race condition in RecordCodeLocation
bitsandfoxes
left a comment
There was a problem hiding this comment.
This looks amazing!
bruno-garcia
left a comment
There was a problem hiding this comment.
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.
| internal void CaptureCodeLocations(CodeLocations codeLocations) | ||
| { | ||
| _options.LogDebug($"Capturing code locations for period: {codeLocations.Timestamp}"); | ||
| CaptureEnvelope(Envelope.FromCodeLocations(codeLocations)); |
There was a problem hiding this comment.
Are code locations not coupled with metrics? In which case they should be on the same envelope ideally
There was a problem hiding this comment.
I think I did that because of this:
https://github.com/getsentry/sentry-python/blob/67c963d9c8d5e7e9de6347aee0edcf0c58d9fb24/sentry_sdk/metrics.py/#L580-L581
Resolves #2880
Dogfooding
Once we have a nuget package, we can use the Symbol Collector to dogfood this.