Conversation
GrzegorzDrozd
commented
Jun 16, 2025
GrzegorzDrozd
commented
- add duration tracking
- add returned rows tracking
- add pending operations metric
- add active connections metric
- minor code fixes
- add duration tracking - add returned rows tracking - add pending operations metric - add active connections metric - minor code fixes
- remove not needed debug code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
============================================
+ Coverage 73.42% 82.41% +8.99%
- Complexity 134 2348 +2214
============================================
Files 11 165 +154
Lines 459 8904 +8445
============================================
+ Hits 337 7338 +7001
- Misses 122 1566 +1444 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@GrzegorzDrozd I've finally had some time to look at metrics generation. I started down the path of tracking everything in pre and post hooks, in #396 - when I got something working I started to realise that I was duplicating a lot of effort from tracing (and some attributes were really hard to get for a metric since you can start a span and add to it while it's in progress, whereas a metric is a single call). Since in my case (an HTTP server span), everything I need seems to be already stored in a span, I tried a different approach (I guess loosely based on span metrics connector and implemented a Span Processor to emit metrics: https://github.com/open-telemetry/opentelemetry-php/pull/1651/files#diff-9ed562697c2c477790ec9394f926eeb261f2433f39fb3eeca3e7999ee7c390dbR54 Looking at your implementation here, I see a similar pattern:
The metrics are closely related to the spans generated nearby. Given that duration and most of the attributes you're interested in are already in a span, do you think you could achieve your goals with a span processor? |
But that would force someone to use spans with metrics? What if they want only to use metrics? What about sampling? So with this argument I think I should move my code outside of hooks with spans and create separate code, so that we would allow using metrics without spans ... |
Yes it would...but we're creating spans anyway (even if they're no-op). Anyway, I'm not saying we have to do it that way, I just want to make sure we need the extra tracking/timing classes before accepting the ongoing maintenance. |
| $parent = Context::getCurrent(); | ||
|
|
||
| $instrumentation->meter() | ||
| ->createUpDownCounter('db.client.connection.count', '1') |
There was a problem hiding this comment.
Should we be creating a new instrument each time, or one that is reused?
There was a problem hiding this comment.
All examples I found always created new meter. I assume this is a cheap operation. Re-using that would be much more complicated I think.
| ): void { | ||
| $parent = Context::getCurrent(); | ||
| $instrumentation->meter() | ||
| ->createHistogram('db.client.operation.duration', 'ms') |
There was a problem hiding this comment.
The db.client.operation.duration metric should use seconds s as the unit.
# Conflicts: # src/Instrumentation/PDO/src/PDOInstrumentation.php
|
Hi @brettmc Question (I am tagging you because you answered in my PR but if you think this is a topic for more wide audience please tag people that need to see it): If you wish I can create a thread in Slack for more interactive discussion. |
Great question. I think it makes sense to include them in the existing instrumentation modules (pdo, postgres, etc). Perhaps initially with an opt-in by env var? |
- code update, dependency update
- fix duration metric
|
@brettmc Please take a look again. |
3 participants
