feat: postgresql responseHook support by nata7che · Pull Request #528 · open-telemetry/opentelemetry-js-contrib

nata7che · 2021-06-09T14:00:17Z

Which problem is this PR solving?

Add the ability to collect the response to a pg query (as an optional configuration). This data can be used for monitoring purposes.

Short description of the changes

add responseHook config to PgInstrumentationConfig
When provided (in addition to the enhancedDatabaseReporting flag, safely use it to collect the data from the execution result

codecov · 2021-06-09T15:30:33Z

Codecov Report

Merging #528 (38b78c6) into main (a52deec) will increase coverage by 0.00%.
The diff coverage is 97.41%.

@@           Coverage Diff            @@
##             main     #528    +/-   ##
========================================
  Coverage   94.92%   94.93%            
========================================
  Files         162      159     -3     
  Lines        9991     9828   -163     
  Branches      992      932    -60     
========================================
- Hits         9484     9330   -154     
+ Misses        507      498     -9

Impacted Files	Coverage Δ
...ntelemetry-instrumentation-pg/test/pg-pool.test.ts	`91.42% <96.15%> (+1.91%)`	⬆️
...e/opentelemetry-instrumentation-pg/test/pg.test.ts	`95.01% <98.00%> (+0.59%)`	⬆️
...elemetry-instrumentation-pg/src/instrumentation.ts	`82.47% <100.00%> (+0.18%)`	⬆️
...node/opentelemetry-instrumentation-pg/src/utils.ts	`97.61% <100.00%> (+0.35%)`	⬆️
...ation-user-interaction/src/enums/AttributeNames.ts
...umentation-user-interaction/src/instrumentation.ts
...ry-instrumentation-user-interaction/src/version.ts

nata7che · 2021-06-09T20:04:46Z

Hey @obecny, fixed the commits issue (had to create a new PR), tnx!

rauno56 · 2021-06-11T08:30:56Z

plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts

@@ -199,6 +200,7 @@ export class PgInstrumentation extends InstrumentationBase {
            .then((result: unknown) => {
              // Return a pass-along promise which ends the span and then goes to user's orig resolvers
              return new Promise(resolve => {


I understand that nested new Promise was here before but it really is not needed.
You can just do the sync calls inside the then callback function and it behaves exactly the same.

thanks @rauno56, I agree and also noticed it when writing this PR. However, and if that is alright with you, I'd prefer to leave this change to a different PR?

plugins/node/opentelemetry-instrumentation-pg/src/types.ts

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

rauno56 · 2021-06-11T08:41:30Z

Generally looks good! Thanks for the effort you've put into it and sorry for the late review!

plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts

blumamir · 2021-06-13T06:59:31Z

plugins/node/opentelemetry-instrumentation-pg/src/types.ts

+import { InstrumentationConfig } from '@opentelemetry/instrumentation';
+
+export interface PgInstrumentationExecutionResponseHook {
+  (span: api.Span, data: pgTypes.QueryResult | pgTypes.QueryArrayResult): void;


Instead of sending data as the second parameter, it might be better to send an responseInfo with the data on it.
That way, when someone wants to add new parameters in the future, the function signature will not grow more and more.

Link to discussion
Example

Good Idea! will change.

blumamir · 2021-06-13T07:15:46Z

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

+      },
+      err => {
+        if (err) {
+          diag.error('Error running response hook', err);


optional: I like to add a prefix to my logs so if it prints, there is context on where it's coming from. Something like: pg instrumentation: ${...}

Great idea, I’d suggest implementing this (in a separate PR, of course) at the InstrumentationBase level so that it won't be necessary to manually add the prefix separately for each instrumentation class. WDYT?

I think it's a great idea :)

I now read the v0.21.0 release notes and looks like it was added there:
open-telemetry/opentelemetry-js#2261

It was indeed added on instrumentation but you should use this._diag from the instrumentation class to be able to use it. I think you'll need to give the instance class here to make it work

@vmarchaud, @blumamir for this PR I recommend not adding this change.
_diag is a protected member, it's accessible only internally within the class or any class that extends it but not externally. Since the pg instrumentation uses the utils module, passing the instrumentation will not be enough and a refactor is needed here.

Yes, I agree. We tried to refactor our instrumentations as well and bumped into the same issue, and decided not to use this component logger after all.
However, we made sure all the log prints are prefixed with ${packageName} instrumentation:, for example.

But I see there is no convention for the contrib instrumentations on that, so it's up to you.

why not passing the component logger to the util class directly from class when you call the util method ? @blumamir @nata7che in worst scenario I would change the signature to be public from protected instead of trying to mimic the behaviour of component diag logger. But it should be possible to simply pass the logger from class to util

blumamir · 2021-06-13T07:18:08Z

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

+  pgResult: pgTypes.QueryResult | pgTypes.QueryArrayResult | unknown
+) {
+  if (
+    config.enhancedDatabaseReporting &&


why do you require truthy enhancedDatabaseReporting for calling responseHook?

will be removed also in accordance to @rauno56's comment above

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

plugins/node/opentelemetry-instrumentation-pg/src/types.ts

plugins/node/opentelemetry-instrumentation-pg/test/pg-pool.test.ts

blumamir · 2021-06-13T07:48:46Z

plugins/node/opentelemetry-instrumentation-pg/test/pg-pool.test.ts

+                0
+              );
+              runCallbackTest(span, pgAttributes, events, unsetStatus, 2, 1);
+              assert.ok(result, 'pool.query() returns a promise');


result here was already awaited, right? so it's not a promise, it's the actual response for the invocation. Not sure what this assertion is meant to test... (but I don't mind if it stays here as well).

blumamir · 2021-06-13T08:05:17Z

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

        message: err.message,
      });
+    } else {
+      handleExecutionResult(instrumentationConfig, span, res);


Shouldn't we also call the response hook in the patchCallbackPGPool function below?
Probably not, as I see there are tests for that, but how come it works?

The PgPool instrumentation added an additional patching to the pgPool.connect method, and uses the same query patching.

I have to admit I'm not very familiar with this instrumentation and its behavior.
So in what cases is "patchCallbackPGPool" being called?

nata7che · 2021-06-21T10:20:06Z

hey @rauno56! I'd love to get your approval if you don't have any further comments 🙏🏻

plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts

MSNev · 2021-06-23T16:41:22Z

plugins/node/opentelemetry-instrumentation-pg/src/utils.ts

+  span: Span,
+  pgResult: pgTypes.QueryResult | pgTypes.QueryArrayResult | unknown
+) {
+  if (config.responseHook !== undefined && pgResult !== undefined) {


Same comment from the other "add responseHook", why explicitly only checking against undefined? Why not if (config.responseHook && pg...)

Or event better if (typeof config.responseHook === "function" && pg...)

I think we should just have if (typeof config.responseHook === "function") if user adds hook we should always run it whether the pgResult is undefined or not as this is also some kind of information for someone. Preventing running this hook in such case will confuse user why the hook didn't run soo I would definitely remove it.

@obecny pushed 👍🏻 good idea!

nata7che · 2021-06-28T06:51:07Z

@obecny @MSNev @vmarchaud thanks for your approval 💪🏻 are we good to go ahead and merge?

rauno56 · 2021-06-28T08:37:31Z

hey @rauno56! I'd love to get your approval if you don't have any further comments 🙏🏻

Hey, @nata7che, sorry for disappearing - I was offline for a week.
My comments weren't meant to be blocking anyways! :) LGTM!

nata7che requested a review from a team June 9, 2021 14:00

rauno56 reviewed Jun 11, 2021

View reviewed changes

blumamir reviewed Jun 13, 2021

View reviewed changes

nata7che requested review from blumamir and rauno56 June 13, 2021 19:38

feat: postgresql responseHook support

3f8cdb0

nata7che force-pushed the main branch from ca1b357 to 3f8cdb0 Compare June 13, 2021 19:49

blumamir approved these changes Jun 14, 2021

View reviewed changes

Merge branch 'main' into main

137f925

nata7che requested a review from vmarchaud June 16, 2021 12:01

Merge branch 'main' into main

e7914ff

vmarchaud approved these changes Jun 20, 2021

View reviewed changes

obecny reviewed Jun 21, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts Outdated Show resolved Hide resolved

nata7che added 2 commits June 22, 2021 09:59

Merge branch 'main' into main

022f0f7

CR comments

5f071f3

nata7che requested a review from obecny June 23, 2021 10:51

MSNev reviewed Jun 23, 2021

View reviewed changes

nata7che added 2 commits June 24, 2021 08:45

Merge branch 'main' into main

0a6952a

CR comments

38b78c6

MSNev approved these changes Jun 24, 2021

View reviewed changes

obecny approved these changes Jun 24, 2021

View reviewed changes

rauno56 approved these changes Jun 28, 2021

View reviewed changes

vmarchaud merged commit a8465a2 into open-telemetry:main Jun 28, 2021

dyladan added the enhancement New feature or request label Jul 2, 2021

Conversation

nata7che commented Jun 9, 2021

Which problem is this PR solving?

Short description of the changes

Uh oh!

codecov bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nata7che commented Jun 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rauno56 commented Jun 11, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blumamir Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obecny Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nata7che commented Jun 21, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nata7che commented Jun 28, 2021

Uh oh!

rauno56 commented Jun 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Jun 9, 2021 •

edited

Loading

blumamir Jun 13, 2021 •

edited

Loading

obecny Jun 24, 2021 •

edited

Loading