Skip to content

Conversation

@GrzegorzDrozd
Copy link
Contributor

…and object

Introduced TimerTrackerById, TimerTrackerByObject, AttributeTrackerById, and AttributeTrackerByObject classes for managing timers and attributes based on id or object references.

This will be used by auto instrumentation and metrics in contrib modules.

…and object

Introduced `TimerTrackerById`, `TimerTrackerByObject`, `AttributeTrackerById`, and `AttributeTrackerByObject` classes for managing timers and attributes based on id or object references.

This will be used by auto instrumentation and metrics in contrib modules.
@GrzegorzDrozd GrzegorzDrozd requested a review from a team as a code owner May 29, 2025 08:48
@bobstrecansky bobstrecansky marked this pull request as draft June 4, 2025 12:05
@brettmc
Copy link
Contributor

brettmc commented Jun 13, 2025

  • I'd like to understand how this would be used by an instrumentation package. The metrics implementation is pretty extensive, does it already have features that can be used? I'd prefer to see a metrics implementation PR in an existing package, using this code locally, before extracting it to a re-usable place
  • would we have a need for metrics to be tracked across different instrumentation packages?
  • instrumentation packages are actively discouraged (per spec) from depending on the SDK

@ChrisLightfootWild
Copy link
Contributor

For any time related operations, there's also already clock functionality in the API - so it might make sense to utilise that over the raw calls to microtime.

@GrzegorzDrozd
Copy link
Contributor Author

@brettmc Usage example is here: open-telemetry/opentelemetry-php-contrib#391

@stale
Copy link

stale bot commented Jul 19, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale This issue didn't have recent activity label Jul 19, 2025
@GrzegorzDrozd
Copy link
Contributor Author

Hi @brettmc , somehow I missed this notification :/

  • I'd like to understand how this would be used by an instrumentation package. The metrics implementation is pretty extensive, does it already have features that can be used? I'd prefer to see a metrics implementation PR in an existing package, using this code locally, before extracting it to a re-usable place

For example: TimerTrackerById would keep timers for everything that sending external requests: PDO, Redis, HttpClient, Curl, Guzzle etc. Everything that needs to get duration.

* would we have a need for metrics to be tracked across different instrumentation packages?

Instance of a tracker would be created in ::register of a given contrib package. It would not be shared.

* instrumentation packages are actively discouraged (per spec) from depending on the SDK

Well we can copy and paste those classes all over but that would also be a bad practice.

Maybe some form of cross contrib code share? We could do some namespace shenanigans and have shared code just copied to a contrib module?
From this:

  "autoload": {
    "psr-4": {
      "OpenTelemetry\\Contrib\\Instrumentation\\PDO\\": "src/"
    },
    "files": [
      "_register.php"
    ]
  },

to

  "autoload": {
    "psr-4": {
      "OpenTelemetry\\Contrib\\Instrumentation\\PDO\\": "src/",
      "OpenTelemetry\\Contrib\\Instrumentation\\Shared\\": "shared/"
    },
    "files": [
      "_register.php"
    ]
  },

Or even put everything into one file and use files + copy into every contrib?

@stale stale bot removed the stale This issue didn't have recent activity label Jul 20, 2025
@brettmc
Copy link
Contributor

brettmc commented Jul 21, 2025

Well we can copy and paste those classes all over but that would also be a bad practice.

Agreed, that sounds awful! The most likely place is its own package in contrib, or in our API. If it goes into API, then under API\Instrumentation feels like the right place - we already have some code there to support auto-instrumentation 🤔

@stale
Copy link

stale bot commented Oct 18, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale This issue didn't have recent activity label Oct 18, 2025
@stale stale bot removed the stale This issue didn't have recent activity label Nov 1, 2025
@codecov
Copy link

codecov bot commented Nov 1, 2025

Codecov Report

❌ Patch coverage is 94.20290% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.47%. Comparing base (90005d5) to head (5ed6fb3).

Files with missing lines Patch % Lines
src/API/Metrics/TimerTrackerById.php 83.33% 2 Missing ⚠️
src/API/Metrics/TimerTrackerByObject.php 84.61% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1614      +/-   ##
============================================
+ Coverage     68.35%   68.47%   +0.12%     
- Complexity     2970     3001      +31     
============================================
  Files           448      452       +4     
  Lines          8712     8781      +69     
============================================
+ Hits           5955     6013      +58     
- Misses         2757     2768      +11     
Flag Coverage Δ
8.1 ?
8.2 ?
8.3 ?
8.4 ?
8.5 68.47% <94.20%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ation/AutoInstrumentation/AttributeTrackerById.php 100.00% <100.00%> (ø)
...n/AutoInstrumentation/AttributeTrackerByObject.php 100.00% <100.00%> (ø)
src/API/Metrics/TimerTrackerById.php 83.33% <83.33%> (ø)
src/API/Metrics/TimerTrackerByObject.php 84.61% <84.61%> (ø)

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90005d5...5ed6fb3. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…and object

- move files around, according to code review
- normalized code, unified method signatures, fixed phpstan comments
- replace microtime with ClockInterface
- fixed unit tests
@GrzegorzDrozd
Copy link
Contributor Author

GrzegorzDrozd commented Nov 1, 2025

@brettmc Code is adjusted: files are moved, tests updated etc. I also replaced raw microtime with ClockInterface. I will adjust linked PR to PDO metrics tomorrow.

…and object

- add duration in seconds for db related operations
@GrzegorzDrozd GrzegorzDrozd marked this pull request as ready for review November 5, 2025 09:16
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.

3 participants