search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
566 stars 203 forks source link

Add logging to Metrics #3324

Closed jamescrosswell closed 2 weeks ago

jamescrosswell commented 2 weeks ago

Resolves https://github.com/getsentry/sentry-dotnet/issues/3310

skip-changelog

jamescrosswell commented 2 weeks ago

@bitsandfoxes I had a bit of a scan through the MetricAggregator class but didn't see anywhere obvious to add logging... what additional logging did you want here?

bitsandfoxes commented 2 weeks ago

@bitsandfoxes I had a bit of a scan through the MetricAggregator class but didn't see anywhere obvious to add logging... what additional logging did you want here?

Initially, I thought that just emitting a metric could leave a log but I can see this getting quite noisy. Then I thought maybe just during flushing but there are already logs there. Now I think the issue came through Unity's logger not being able to handle the logs coming from the background.

jamescrosswell commented 2 weeks ago

@bitsandfoxes I had a bit of a scan through the MetricAggregator class but didn't see anywhere obvious to add logging... what additional logging did you want here?

Initially, I thought that just emitting a metric could leave a log but I can see this getting quite noisy. Then I thought maybe just during flushing but there are already logs there. Now I think the issue came through Unity's logger not being able to handle the logs coming from the background.

OK, should we bother with this PR or close it do you think? It just adds a log if metrics are disabled... not sure how useful that is.

We'll need to fix the broken verify tests if we want to merge.

bitsandfoxes commented 2 weeks ago

It just adds a log if metrics are disabled

How about we do it the other way around and treat it like an integration that logs if it got registered.