feat: Add metrics support#1151
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1151 +/- ##
==========================================
- Coverage 86.04% 85.46% -0.58%
==========================================
Files 62 67 +5
Lines 6090 6509 +419
==========================================
+ Hits 5240 5563 +323
- Misses 635 708 +73
- Partials 215 238 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
345f1c0 to
8ee32f7
Compare
interfaces.go
Outdated
| // Count records a count metric. | ||
| Count(name string, count int64, options MeterOptions) | ||
| // Gauge records a gauge metric. | ||
| Gauge(name string, value float64, options MeterOptions) |
There was a problem hiding this comment.
@giortzisg Gauge can accept normal integers (non-floats), that's why I created the Numbers interface above.
There was a problem hiding this comment.
I know, but the generic solution didn't make much sense. It didn't apply to Count and Dist and in most cases you would need float64 anyways for most calculations, so it is extra verbose for no reason. Type casting in some cases is fine, but I might introduce also a GaugeInt and some other convenience methods.
There was a problem hiding this comment.
The convenience methods would be nice!
There was a problem hiding this comment.
On another note, I was thinking it's because of JSON serialization for float64 and int differences.
There was a problem hiding this comment.
JSON serialization for float64 and int differences.
That still is a problem, but doesn't get solved by generics. Count should be an int and Gauge/Dist floats, so the problem is storing and converting without precision loss between count/gauge/dist metric events.
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Internal Changes 🔧Release
Other
🤖 This preview updates automatically when you update the PR. |
The previous batch_processor implementation needed to stop and read the timer so that any stale data was cleared. From go1.23 and on, the time.Timer uses an unbuffered channel, meaning that we can remove both reading and stop(), which simplifies the batch_processor significantly.
| sentryhttp "github.com/getsentry/sentry-go/http" | ||
| ) | ||
|
|
||
| var meter sentry.Meter |
There was a problem hiding this comment.
Nooo don't do this. It looks a lot like otel.
| "github.com/getsentry/sentry-go/internal/ratelimit" | ||
| ) | ||
|
|
||
| const errorType = "" |
There was a problem hiding this comment.
What is this for? Providing comment would be great.
There was a problem hiding this comment.
the errorType if an empty string if you have a look at how the sdk currently works. Just added the type so that it makes more sense.
| name: "metric event", | ||
| event: &Event{ | ||
| EventID: "12345678901234567890123456789012", | ||
| Type: "trace_metric", | ||
| Metrics: []Metric{ | ||
| { | ||
| Name: "test.metric", | ||
| Value: Int64MetricValue(42), | ||
| Timestamp: time.Date(2023, 1, 1, 12, 0, 0, 0, time.UTC), | ||
| Type: MetricTypeCounter, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
We should also test for invalid and nil MetricValue, as well as invalid metric type (other than count, dist, gauge)
| // If there are duplicate keys, the value from the last map takes precedence. | ||
| // | ||
| // CC BY-SA 4.0 Oliver (https://stackoverflow.com/a/74750675/3153224) | ||
| func MergeMaps[M ~map[K]V, K comparable, V any](src ...M) M { |
There was a problem hiding this comment.
We can safely remove this if this is not used.
| // Duration Units. | ||
| const ( | ||
| UnitNanosecond = "nanosecond" | ||
| UnitMicrosecond = "microsecond" | ||
| UnitMillisecond = "millisecond" | ||
| UnitSecond = "second" | ||
| UnitMinute = "minute" | ||
| UnitHour = "hour" | ||
| UnitDay = "day" | ||
| UnitWeek = "week" | ||
| ) |
There was a problem hiding this comment.
I would prefer to have this as a type of it's own, better for static typing.
There was a problem hiding this comment.
We want to receive custom units as string, but also allow the users to use the supported ones like that, so can't really have a custom type for it.
There was a problem hiding this comment.
So the freedom of type/unit is intentional?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1151 +/- ##
==========================================
- Coverage 86.04% 85.45% -0.60%
==========================================
Files 62 67 +5
Lines 6090 6509 +419
==========================================
+ Hits 5240 5562 +322
- Misses 635 709 +74
- Partials 215 238 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Description
Implement metrics based on https://develop.sentry.dev/sdk/telemetry/metrics/ and https://develop.sentry.dev/sdk/data-model/envelope-items/#trace-metric
Issues
Usage example
Changelog Entry