feat(instrumentation-pg): add post-query custom attributes hook for spans #767

Alon-Katz · 2021-11-30T13:43:47Z

Which problem is this PR solving?

There is currently no way to add custom attributes to Postgres spans before the query is resolved

Short description of the changes

add postQueryHook config to PgInstrumentationConfig
When provided safely use it to add custom attributes to Postgres spans
Fixes Copy post-query custom attributes hook from pg plugin to pg instrumentation #363

linux-foundation-easycla · 2021-11-30T13:45:29Z

❌ The commit (105e837 ,568c1be0b29747f2cc725cd6abb7808821697198) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

Alon-Katz · 2021-11-30T14:34:01Z

/easycla

codecov · 2021-11-30T14:47:12Z

Codecov Report

Merging #767 (ae57bd8) into main (81b3190) will decrease coverage by 1.75%.
The diff coverage is 93.97%.

❗ Current head ae57bd8 differs from pull request most recent head f724864. Consider uploading reports for the commit f724864 to get more accurate results

@@            Coverage Diff             @@
##             main     #767      +/-   ##
==========================================
- Coverage   94.88%   93.13%   -1.76%     
==========================================
  Files          13       10       -3     
  Lines         684      888     +204     
  Branches      138       94      -44     
==========================================
+ Hits          649      827     +178     
- Misses         35       61      +26

Impacted Files	Coverage Δ
...node/opentelemetry-instrumentation-pg/src/utils.ts	`96.77% <88.88%> (ø)`
...e/opentelemetry-instrumentation-pg/test/pg.test.ts	`94.13% <94.11%> (ø)`
...elemetry-instrumentation-pg/src/instrumentation.ts	`86.27% <100.00%> (ø)`
...y-instrumentation-long-task/src/instrumentation.ts
...metry-instrumentation-document-load/src/version.ts
...lemetry-instrumentation-document-load/src/utils.ts
...telemetry-instrumentation-long-task/src/version.ts
...entelemetry-instrumentation-long-task/test/util.ts
...lugin-react-load/src/BaseOpenTelemetryComponent.ts
...entation-document-load/src/enums/AttributeNames.ts
... and 16 more

vmarchaud · 2021-12-05T14:32:58Z

@Alon-Katz You'll need to rebase your PR against main

Alon-Katz · 2021-12-19T12:24:42Z

@vmarchaud Sorry for the delay, I have rebased against main.

vmarchaud · 2021-12-19T13:32:23Z

@rauno56 could you take the time to review this PR ?

rauno56 · 2021-12-20T10:25:10Z

I apologize for missing this one, @Alon-Katz. 20 days is really an unacceptable delay for a review.

This is no doubt an useful feature. Not only for pg but potentially every other instrumentation.

What kind of custom attributes are you looking to add to the spans from pg, @Alon-Katz?
From the test cases in the PR I'm guessing it's about doing additional processing on what's already in the attributes and trying to provide additional convenience filtering to the o11y suite you are using?

vmarchaud · 2021-12-20T13:16:27Z

@rauno56 Judging from the fact that the PR has been opened from https://github.com/epsagon (which is an apm product) i guess they want to have more metadata to display.

rauno56 · 2021-12-20T13:48:13Z

Sure, I was asking to validate whether it could be done somewhere else in the pipeline. It's a functionality commonly asked for - it doesn't scale well to add post-hooks to each of the instrumentations.
Currently the span processor provides a onEnd hook to a ReadableSpan, which, is not sufficient in this case - we should either reconsider that or add something into the instrumentation base.

vmarchaud · 2021-12-20T15:00:51Z

we should either reconsider that or add something into the instrumentation base.

There has been multiple discussions across different instrumentations to do this on the instrument base, the problem is that you generally want instrumentation specific context so using SpanProcessor isn't viable. While no one took the work to add this kind of mechanism on instrumentation base its okay for me to let people add those hooks in each instrumentation

rauno56 · 2021-12-20T16:19:07Z

What to you mean by instrumentation specific context? If I understand you correctly, I can only come up with

the configuration for the instrumentation;
information that's already on the span.

vmarchaud · 2021-12-20T17:05:03Z

What to you mean by instrumentation specific context

Not every information is available as span attributes, for example one might want to add remote server version but we don't have it by default. I don't think its scalable to try add all data by default and let people use span processors. Specially since vendor might want other kind of data that we didn't think of.
I'm not sure to see why it's important anyway ? We already have those hooks in other instrumentation (so it solves a problem) and while there is no system on the base instrumentation class we can still merge this PR to allow this kind of use case ?

rauno56 · 2021-12-21T10:37:35Z

Not every information is available as span attributes, for example one might want to add remote server version but we don't have it by default. I don't think its scalable to try add all data by default and let people use span processors. Specially since vendor might want other kind of data that we didn't think of.

We agree on that - There has to be a mechanism for vendors or users to add their own attributes and this PR solves for that in the context of pg instrumentation.

However, now that we've seen the same feature added into different instrumentations with somewhat different semantics or ergonomics again and again we should know better. I think merging this does more harm than good long term and support solving this in a more general manner instead.

Do you think there's an usecase we cannot support by making it a general feature?

vmarchaud · 2021-12-21T12:56:57Z

Do you think there's an usecase we cannot support by making it a general feature?

I agree that we need to make it a general feature, but i don't agree with refusing PRs because contributors don't have the time to make a generic system.

cc @dyladan if you could give your opinion on this to see what to do here

dyladan · 2021-12-21T16:04:13Z

I think as a feature it makes sense. There will always be information that people want to add to the spans with additional context that is not available at the span processor level. @rauno56 is the owner of this module and I think he should have the final decision. When we decided to give ownership of modules to the contributors who maintain them, the original idea was that they would have the final say in decisions like this unless there was a security concern or similar extenuating circumstance. My personal suggestion would be to accept the feature because there is currently no generic system and the feature provides value, but ultimately the decision is Rauno's.

Creating a generic system for this feature might be difficult because of all the different types of modules involved, but maybe we could at least make the configurations consistent across instrumentations that hook similar modules. For example, all the DB instrumentations would probably benefit from the same hooks with consistent naming. It might make sense for us to make our own document in this repo which describes a set of suggested hooks for each type of module?

rauno56 · 2021-12-27T17:47:26Z

I agree that we need to make it a general feature, but i don't agree with refusing PRs because contributors don't have the time to make a generic system.

Yes, that makes sense actually, @vmarchaud. My bad. @Alon-Katz, could you please enable "Allow edits from maintainers" so we wouldn't be stuck with the branch being out of date with the base branch.

In conclusion, I'd love to merge this for now with the hope to replace it with something more general in the long run.

I do not understand, however, why do you find it difficult to make something more general?

dyladan · 2021-12-27T17:51:02Z

I do not understand, however, why do you find it difficult to make something more general?

Simply because of the wide variety of modules that need to be instrumented and the resulting variety of useful hook points and their arguments. Maybe its easier than I'm thinking.

vmarchaud · 2021-12-27T19:32:37Z

I do not understand, however, why do you find it difficult to make something more general?

I don't think there is a lot of hidden complixity in this (not saying its trivial though) but we all have limited time to allocate and this wasn't a priority in the past (specially with the GA of api/sdk). There was discussion for a more generic instrumentation API that might have specified how to handle generic configuration like this (open-telemetry/opentelemetry-js#2359), so generally we might want to have a complete API for instrumentation before trying to implement this feature in the base instrumentation class

Alon-Katz · 2021-12-28T11:49:56Z

Yes, that makes sense actually, @vmarchaud. My bad. @Alon-Katz, could you please enable "Allow edits from maintainers" so we wouldn't be stuck with the branch being out of date with the base branch.

@rauno56 I followed the link for "Allow edits from maintainers", but for some reason it seems like I do not have this option. According to the documentation, the option should be in the right section below the notifications, this is how it looks on my side:

Flarna · 2021-12-28T11:54:58Z

@Alon-Katz Most likely the problem is because the repo you use is not your personal fork, it seems to be a fork from an organization. For such forks it's not working to allow maintainers to edit via this checkbox.

So either move to a private fork in future PRs or take the burden to merge master regularly...

I think @dyladan found some rest call from github to enable this for a single branch/PR even on org forks.

dyladan · 2021-12-28T14:00:39Z

You can do it with cURL:

curl -X PATCH -u $GITHUB_USER:$GITHUB_TOKEN -H "Accept: application/vnd.github.v3+json" -H "Content-Type: application/vnd.github.v3+json" -d "{\"maintainer_can_modify\": true}"  "https://api.github.com/repos/open-telemetry/opentelemetry-js-contrib/pulls/767"

Alon-Katz · 2021-12-28T14:12:20Z

You can do it with cURL:

curl -X PATCH -u $GITHUB_USER:$GITHUB_TOKEN -H "Accept: application/vnd.github.v3+json" -H "Content-Type: application/vnd.github.v3+json" -d "{\"maintainer_can_modify\": true}"  "https://api.github.com/repos/open-telemetry/opentelemetry-js-contrib/pulls/767"

Thanks! I ran the command, hopefully it works.

linux-foundation-easycla · 2022-01-06T13:51:12Z

✅ Alon Katz (d6f8af0, 85d5611, 27a4050)
✅ Rauno Viskus (f724864)

❌ The commit (07924d6) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

rauno56 · 2022-01-07T09:54:53Z

* white_check_mark  Alon Katz ([d6f8af0](https://github.com/open-telemetry/opentelemetry-js-contrib/commit/d6f8af008163a72c8b590ae002a7c780aab5b866), [85d5611](https://github.com/open-telemetry/opentelemetry-js-contrib/commit/85d561152a84e0e63b094f64ae7ab958130ba63b), [27a4050](https://github.com/open-telemetry/opentelemetry-js-contrib/commit/27a405076c3bf2214a6d8d9f34227a73e8b8cde1))

* white_check_mark  Rauno Viskus ([f724864](https://github.com/open-telemetry/opentelemetry-js-contrib/commit/f724864f9ac08e07a85406937f37f1c809d6db4c))


* x The commit ([07924d6](https://github.com/open-telemetry/opentelemetry-js-contrib/commit/07924d68c84e601d308ac19b4369c3c6d3271497)) is missing the User's ID, preventing the EasyCLA check. [Consult GitHub Help](https://help.github.com/en/github/committing-changes-to-your-project/why-are-my-commits-linked-to-the-wrong-user) to resolve.For further assistance with EasyCLA, [please submit a support request ticket](https://jira.linuxfoundation.org/servicedesk/customer/portal/4).

@Alon-Katz seems like your CLA is signed for the Cisco email, but one of the commits(07924d6) is done from an Epsagon email address. That's why EasyCLA is probably upset.

I'll review the PR on Monday.

blumamir · 2022-01-09T20:22:27Z

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

+}
+
+export interface PgPostQueryHookFunction {
+  (ctx: QueryContext): void;


Can you please change the hook signature to follow the other hook example in this file:

export interface PgInstrumentationExecutionResponseHook { (span: api.Span, responseInfo: PgResponseHookInformation): void; }

First parameter should be the span.
Second parameter is called Info on the other hook. Could be nice in my opinion to call it queryInfo (to stay consistent with the existing name.
From what I remember, this is also the convention in other instrumentations.

blumamir · 2022-01-09T20:43:09Z

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

+          assert.strictEqual(ctx.query, query);
+          assert.strictEqual(ctx.params, undefined);


These assertions are performed in the hook body.
I think that if they fail, the exception will just be silently ignored and test will pass.
That is because the hook is protected with safeExecuteInTheMiddle which does not propagate exceptions to the caller.

blumamir · 2022-01-09T20:53:07Z

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

+        }
+      });
+      assert.strictEqual(called, true);
+    });


I also suggest having some negative tests for the hook - what happens if it throws? what if it's not a function?
This introduced new logic is currently not covered with tests.
You can see an example of that on the responseHook tests here

blumamir · 2022-01-09T20:57:50Z

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

@@ -563,5 +563,113 @@ describe('pg', () => {
        client.query('SELECT NOW()').then(queryHandler);
      });
    });
+
+    it('should call postQueryHook with query text if set', async () => {


You can wrap this section with a describe to group all these tests under postQueryHook.
This will also be consistent with the existing tests for responseHook see here

blumamir · 2022-02-02T22:00:41Z

Hi @Alon-Katz
Still interested in this PR?

Alon-Katz · 2022-02-03T08:08:49Z

Hi @blumamir, thanks for time you put into making the CR but unfortunately I will not have the time to continue this PR at the moment, I will close it.

Alon-Katz requested a review from a team as a code owner November 30, 2021 13:43

This comment has been minimized.

Sign in to view

vmarchaud approved these changes Dec 5, 2021

View reviewed changes

vmarchaud requested a review from a team December 5, 2021 14:33

Alon-Katz added 3 commits December 19, 2021 14:04

add post-query custom attributes hook for spans

d6f8af0

Improve handlePostQueryHook readability

85d5611

Improve readability

27a4050

github-actions bot assigned blumamir, legendecas, naseemkullah, NathanielRN, obecny, rauno56 and willarmiros Dec 19, 2021

Alon-Katz force-pushed the add-post-query-attributes-hook branch from f24b996 to 27a4050 Compare December 19, 2021 12:23

github-actions bot requested review from rauno56 and removed request for a team December 19, 2021 12:23

Merge branch 'main' into add-post-query-attributes-hook

07924d6

Merge branch 'main' into add-post-query-attributes-hook

f724864

rauno56 mentioned this pull request Jan 7, 2022

Include client ID in pg query span #280

Closed

blumamir reviewed Jan 9, 2022

View reviewed changes

Alon-Katz closed this Feb 3, 2022

Flarna mentioned this pull request Apr 12, 2023

chore(detectors/node): adapted breaking change in @opentelemetry/sdk-node@0.37.0 #1462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instrumentation-pg): add post-query custom attributes hook for spans #767

feat(instrumentation-pg): add post-query custom attributes hook for spans #767

Alon-Katz commented Nov 30, 2021 •

edited

This comment has been minimized.

linux-foundation-easycla bot commented Nov 30, 2021

This comment has been minimized.

Alon-Katz commented Nov 30, 2021

codecov bot commented Nov 30, 2021 •

edited

vmarchaud commented Dec 5, 2021

Alon-Katz commented Dec 19, 2021

vmarchaud commented Dec 19, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 21, 2021

vmarchaud commented Dec 21, 2021 •

edited

dyladan commented Dec 21, 2021

rauno56 commented Dec 27, 2021

dyladan commented Dec 27, 2021

vmarchaud commented Dec 27, 2021

Alon-Katz commented Dec 28, 2021

Flarna commented Dec 28, 2021 •

edited

dyladan commented Dec 28, 2021 •

edited

Alon-Katz commented Dec 28, 2021

linux-foundation-easycla bot commented Jan 6, 2022

rauno56 commented Jan 7, 2022

blumamir Jan 9, 2022

blumamir Jan 9, 2022

blumamir Jan 9, 2022

blumamir Jan 9, 2022

blumamir commented Feb 2, 2022

Alon-Katz commented Feb 3, 2022

		assert.strictEqual(ctx.query, query);
		assert.strictEqual(ctx.params, undefined);

feat(instrumentation-pg): add post-query custom attributes hook for spans #767

feat(instrumentation-pg): add post-query custom attributes hook for spans #767

Conversation

Alon-Katz commented Nov 30, 2021 • edited

Which problem is this PR solving?

Short description of the changes

This comment has been minimized.

linux-foundation-easycla bot commented Nov 30, 2021

This comment has been minimized.

Alon-Katz commented Nov 30, 2021

codecov bot commented Nov 30, 2021 • edited

Codecov Report

vmarchaud commented Dec 5, 2021

Alon-Katz commented Dec 19, 2021

vmarchaud commented Dec 19, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 20, 2021

vmarchaud commented Dec 20, 2021

rauno56 commented Dec 21, 2021

vmarchaud commented Dec 21, 2021 • edited

dyladan commented Dec 21, 2021

rauno56 commented Dec 27, 2021

dyladan commented Dec 27, 2021

vmarchaud commented Dec 27, 2021

Alon-Katz commented Dec 28, 2021

Flarna commented Dec 28, 2021 • edited

dyladan commented Dec 28, 2021 • edited

Alon-Katz commented Dec 28, 2021

linux-foundation-easycla bot commented Jan 6, 2022

rauno56 commented Jan 7, 2022

blumamir Jan 9, 2022

Choose a reason for hiding this comment

blumamir Jan 9, 2022

Choose a reason for hiding this comment

blumamir Jan 9, 2022

Choose a reason for hiding this comment

blumamir Jan 9, 2022

Choose a reason for hiding this comment

blumamir commented Feb 2, 2022

Alon-Katz commented Feb 3, 2022

Alon-Katz commented Nov 30, 2021 •

edited

codecov bot commented Nov 30, 2021 •

edited

vmarchaud commented Dec 21, 2021 •

edited

Flarna commented Dec 28, 2021 •

edited

dyladan commented Dec 28, 2021 •

edited