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

fix(instrumentation-pg): ensure db.client.operation.duration metric is recorded for Promises API usage of pg #2480

Merged

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Oct 11, 2024

Refs: #2380


While working on pg tests in #2464 I accidentally noticed that the duration metric added to instr-pg in #2380 didn't work if the Promises API of the pg client was used.

@trentm trentm self-assigned this Oct 11, 2024
@trentm trentm requested a review from a team as a code owner October 11, 2024 20:47
@trentm trentm requested a review from maryliag October 11, 2024 20:47
@@ -277,6 +277,7 @@ describe('pg-pool', () => {
await client.query('SELECT NOW()');
} finally {
client.release();
await newPool.end();
Copy link
Contributor Author

@trentm trentm Oct 11, 2024

Choose a reason for hiding this comment

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

reviewer note: This is an unrelated change. It fixes a few-second hang in the test file completing (presumably on a pool connection timeout).

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.85%. Comparing base (25e53d6) to head (67ac071).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...elemetry-instrumentation-pg/src/instrumentation.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2480      +/-   ##
==========================================
- Coverage   90.86%   90.85%   -0.02%     
==========================================
  Files         159      159              
  Lines        7849     7851       +2     
  Branches     1621     1621              
==========================================
+ Hits         7132     7133       +1     
- Misses        717      718       +1     
Files with missing lines Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 89.72% <50.00%> (-0.44%) ⬇️

@trentm
Copy link
Contributor Author

trentm commented Oct 11, 2024

The codecov issue is that the recordDuration() call in the promise rejection case wasn't covered. Meh.

const metrics = resourceMetrics.scopeMetrics[0].metrics;
assert.strictEqual(
metrics[0].descriptor.name,
'db.client.operation.duration'
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in commit 2120edf

@maryliag
Copy link
Contributor

That was an awesome catch! Thanks for finding and fixing it!

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

a little nit, otherwise LGTM

@trentm trentm enabled auto-merge (squash) October 15, 2024 23:25
@trentm trentm disabled auto-merge October 22, 2024 15:53
@trentm trentm enabled auto-merge (squash) October 23, 2024 21:08
@trentm trentm merged commit 97a2956 into open-telemetry:main Oct 23, 2024
4 checks passed
@trentm trentm deleted the tm-pg-duration-metric-with-promises branch October 23, 2024 21:08
@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.

3 participants