chore: Added supportability metrics to OTEL metrics API #3164
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3164 +/- ##
==========================================
- Coverage 97.62% 97.59% -0.04%
==========================================
Files 339 339
Lines 50987 51018 +31
==========================================
+ Hits 49777 49791 +14
- Misses 1210 1227 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -44,6 +44,37 @@ function bootstrapOtelMetrics(agent) { | |||
| resource | |||
| }) | |||
|
|
|||
| const getMeter = provider.getMeter | |||
| provider.getMeter = function nrGetMeter(...args) { | |||
There was a problem hiding this comment.
Any reason not to use shim or shimmer to wrap function?
There was a problem hiding this comment.
Reading through (lib/shimmer.js).wrapMethod, I can see that it would be possible to use it here. But if feels rather "overkill"-ish and more designed for instrumenting the wrapped things than what this wrapping is doing. Here, we are merely augmenting the existing functionality by recording metrics.
But I'm also seeing that lib/shimmer.js relies on our otel setup code:
Line 26 in 9bafd77
The actual usage has no impact on wrapMethod, but it would introduce a weird circular dependency.
Ultimately, I don't think this simple case requires all of the plumbing that comes with our shimmer wrapping tool. Do you agree?
There was a problem hiding this comment.
It's probably fine. We can always change later. Don't forget to add these metrics to angler
github-project-automation
bot
moved this from Needs PR Review
to Done: Issues recently completed
in Node.js Engineering Board
This PR resolves #3161.