Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(instrumentation-pg): implementation of metric operation duration #2380

Merged
merged 27 commits into from
Sep 25, 2024

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Aug 13, 2024

Which problem is this PR solving?

Short description of the changes

This PR does not contain all attributes, because all the ones that require parsing of the query will be more complex and need dedicated testing for performance and be opt-in, so I will leave those for the next PR

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.77%. Comparing base (dfb2dff) to head (fa0b0fd).
Report is 245 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2380      +/-   ##
==========================================
- Coverage   90.97%   90.77%   -0.21%     
==========================================
  Files         146      156      +10     
  Lines        7492     7716     +224     
  Branches     1502     1584      +82     
==========================================
+ Hits         6816     7004     +188     
- Misses        676      712      +36     
Files with missing lines Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 89.65% <100.00%> (+1.68%) ⬆️
...node/opentelemetry-instrumentation-pg/src/utils.ts 97.63% <100.00%> (+0.66%) ⬆️

... and 74 files with indirect coverage changes

Copy link
Contributor

@david-luna david-luna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maryliag thanks for your contribution :)

PR looks very good. I left one question I'm curious about before giving approval.

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JacksonWeber JacksonWeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maryliag
Copy link
Contributor Author

@pichlermarc friendly ping if this can be merged

@pichlermarc
Copy link
Member

Failing tests are unrelated, looks like some issue with the runners - I'll merge this in once they work again 🙂

@maryliag maryliag requested a review from a team as a code owner September 23, 2024 13:23
@pichlermarc
Copy link
Member

pichlermarc commented Sep 23, 2024

Failing tests are actually caused by #2445 - I'm not sure how to fix that yet. 😞
Edit: #2446 should fix it 🙂

@pichlermarc
Copy link
Member

The test failures now seem related to the previous PR #2349 which seems to have introduced some flakiness in the tests.

I've re-started, but it's something that we need to address.

@pichlermarc pichlermarc merged commit 050fee0 into open-telemetry:main Sep 25, 2024
21 checks passed
@maryliag maryliag deleted the db-metric-duration branch September 25, 2024 15:48
@dyladan dyladan mentioned this pull request Sep 23, 2024
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Oct 11, 2024
trentm added a commit that referenced this pull request Oct 23, 2024
@dyladan dyladan mentioned this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants