Skip to content

Conversation

@tustanivsky
Copy link
Collaborator

@tustanivsky tustanivsky commented May 6, 2024

This PR introduces the Metrics API for sentry-native.

Current progress:

  • API for creating, enriching and capturing metrics
  • Background metrics aggregation
  • statsd-encoding for envelope items
  • Periodical metrics flushing
  • Normalization for envelope items

Related to #953

@tustanivsky tustanivsky requested a review from supervacuus May 30, 2024 07:13
@supervacuus
Copy link
Collaborator

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

@tustanivsky
Copy link
Collaborator Author

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

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

@supervacuus
Copy link
Collaborator

supervacuus commented Jun 3, 2024

@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. 🤞

The sentry metrics normalization explicitly mentions not using `\w` in their regex-spec because they exclude diacritics.

It seems Windows asserts in isalnum() on chars outside [-1, 127] and we should ensure that before we pass the char on.
@cleptric
Copy link
Member

cleptric commented Jun 3, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

@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 #985 (comment), 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

@JoshuaMoelans
Copy link
Member

Closing this PR since Metrics have been deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants