feat: add OpenTelemetry metrics middleware alongside go-metrics - #531
feat: add OpenTelemetry metrics middleware alongside go-metrics#531adityakrmishra wants to merge 1 commit into
Conversation
|
Thanks for your interest in palantir/go-githubapp, @adityakrmishra! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
Generate changelog in
|
4001868 to
c7aceb2
Compare
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| type: Feature | |||
| description: "feat: add OpenTelemetry metrics middleware alongside go-metrics" | |||
There was a problem hiding this comment.
Remove this file please, its not necessary
c7aceb2 to
22ec341
Compare
jamestoyer
left a comment
jamestoyer
left a comment
There was a problem hiding this comment.
I have some minor comments but overall this seems like a nice addition. Structurally, I think I'd like to see them in a separate directory for now so it's clear it's not part of the regular API and may be changed at a later point. That may happen as we look in to whether we should adopt OTel more widely and have opinions on how it should laid out.
| OTelMetricsKeyHandlerError = "github.handler.errors" | ||
| OTelMetricsKeyDroppedEvents = "github.event.dropped" | ||
| OTelMetricsKeyEventAge = "github.event.age" |
There was a problem hiding this comment.
These seem like new metrics. What is their purpose here? I think I'd want to start with parity with the existing metrics and then introduce anything new in a subsequent PR.
There was a problem hiding this comment.
Done. Removed these entirely to maintain strict 1:1 parity with the existing metrics. We can evaluate adding new telemetry in a future, separate PR.
|
|
||
| return func(next http.RoundTripper) http.RoundTripper { | ||
| return roundTripperFunc(func(r *http.Request) (*http.Response, error) { | ||
| installationID, _ := r.Context().Value(installationKey).(int64) |
There was a problem hiding this comment.
Should we guard against it not existing and explicitly set it to 0 so it's the same as
go-githubapp/githubapp/middleware.go
Lines 59 to 62 in 59ba9f7
There was a problem hiding this comment.
Added the explicit guard and fallback to 0 to exactly match the behavior in middleware.go.
There was a problem hiding this comment.
Could you make the changes in this file in a separate PR rather than conflating it with the OTel work?
There was a problem hiding this comment.
Absolutely. I've force-pushed this branch to remove those changes, and opened them in a clean, dedicated PR here: #593. Apologies for the git mix-up!
a8a50fe to
22ec341
Compare
22ec341 to
ac741f4
Compare
|
Thanks for the great feedback, @jamestoyer! I've updated the PR to address all your points:
(Regarding the Let me know if there's anything else you'd like adjusted! |
4 participants
Addresses #439
Hi team,
Following up on the discussion in #439, this PR introduces OpenTelemetry as an alternative metrics layer. It runs entirely parallel to the existing
rcrowley/go-metricsimplementation to ensure zero disruption to internal Palantir systems that rely on the legacy metrics.What this solves:
The legacy
go-metricslacks label/tag support, forcing high-cardinality data likeinstallation_idinto the metric names. This new OTel middleware maps the exact same data points but uses proper OTel attributes (e.g.,github.rate.remaining{installation.id="42"}).Changes:
go.opentelemetry.io/otel/metrictogo.mod(API only, keeping it lightweight without SDK bloat).githubapp/middleware_otel.gocontaining the parallel implementation.middleware.go,dispatcher.go,scheduler.go).New Entry Points:
Users can now opt-in via:
OTelClientMetrics(mp)OTelErrorCallback(mp)OTelAsyncErrorCallback(mp)WrapSchedulerWithOTel(inner, mp)All existing tests pass, and the code has been written to match the idiomatic style and comment density of the current project. Let me know if any adjustments are needed!