getsentry / sentry-native

Sentry SDK for C, C++ and native applications.
MIT License
378 stars 164 forks source link

Add Metrics API #985

Open tustanivsky opened 1 month ago

tustanivsky commented 1 month ago

This PR introduces the Metrics API for sentry-native.

Current progress:

Related to #953

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 86.35607% with 82 lines in your changes are missing coverage. Please review.

Project coverage is 84.09%. Comparing base (235f837) to head (d65b93a). Report is 5 commits behind head on master.

:exclamation: Current head d65b93a differs from pull request most recent head c52e3c7

Please upload reports for the commit c52e3c7 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #985 +/- ## ========================================== - Coverage 86.53% 84.09% -2.44% ========================================== Files 40 55 +15 Lines 3736 6044 +2308 Branches 0 1315 +1315 ========================================== + Hits 3233 5083 +1850 - Misses 503 841 +338 - Partials 0 120 +120 ```
supervacuus commented 1 month ago

@tustanivsky, do you already understand why the Windows tests time out?

tustanivsky commented 1 month ago

@tustanivsky, do you already understand why the Windows tests time out?

Not really, things work as expected on Windows when running tests locally.

supervacuus commented 1 month ago

@tustanivsky, do you already understand why the Windows tests time out?

Not really, things work as expected on Windows when running tests locally.

I think it happens because a debug assertion failed, and we are stuck in a dialog on the CI runner. Will investigate.

Since it is also stuck in metics_name_sanitize, it might hint at some sanitizer assumptions that also affect the Android build. 🤞

cleptric commented 1 month ago

As we're fundamentally shifting how Metrics will work at Sentry, I suggest putting this PR on hold for now. At least until we know how the next steps look like.

supervacuus commented 1 month ago

As we're fundamentally shifting how Metrics will work at Sentry, I suggest putting this PR on hold for now. At least until we know how the next steps look like,

Thanks for the heads up, @cleptric.

supervacuus commented 1 month ago

@tustanivsky, I would approve this PR in its current state. At least I don't see any blockers for a merge. But given the news in https://github.com/getsentry/sentry-native/pull/985#issuecomment-2145328051, I will keep merging blocked.

When this gets unblocked, I would appreciate it if @markushi could also give this a quick review so we have the perspective of another metrics implementer.

CC: @kahest