feat(pg): add requireParentSpan option

[ryan-codaio]

Which problem is this PR solving?

The current pg autoinstrumentation creates spans for any postgres connection or query, even if this is not part of an active context. This generates a large number of extraneous spans that we don't care about, and opentelemetry currently doesn't have good support for filtering these out.

Short description of the changes

This PR adds a requireParentTrace configuration option that will prevent the autoinstrumentation from generating root-level traces. Similar options already exist in instrumentation-http, instrumentation-ioredis, instrumentation-mongoose, and others.

The implementation is based on the implementation in opentelemetry-instrumentation-http.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ryan-codaio (f0dc26d)
• ✅ login: dyladan / name: Daniel Dyla (369af85)

[henrinormak]

I accidentally duplicated the feature (with a slightly different implementation) in #1211, I closed that one and am willing to help with this one, how can I be useful?

From just the looks of it, the code changes make sense, if anything, maybe the test coverage could be better for requireParentTrace: false case? Similarly, would be good to test that pg-pool does not create spans either, i.e that even without querying, connecting does not create a span?

[dyladan]

Looks like the CLA isn't signed. Please sign it.

how can I be useful?

code reviews are always helpful

[codecov]

Codecov Report

Merging #1199 (22d2b4e) into main (5da46ef) will decrease coverage by 1.43%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1199      +/-   ##
==========================================
- Coverage   96.08%   94.64%   -1.44%     
==========================================
  Files          14        4      -10     
  Lines         893      280     -613     
  Branches      191       44     -147     
==========================================
- Hits          858      265     -593     
+ Misses         35       15      -20     

Impacted Files	Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts	90.16% <100.00%> (ø)
...node/opentelemetry-instrumentation-pg/src/utils.ts	97.95% <100.00%> (ø)
...etry-plugin-react-load/src/enums/AttributeNames.ts
...erator-aws-xray/src/internal/xray-id-generation.ts
...etapackages/auto-instrumentations-web/src/utils.ts
...umentation-user-interaction/src/instrumentation.ts
...strumentation-document-load/src/instrumentation.ts
...entation-document-load/src/enums/AttributeNames.ts
...emetry-propagator-instana/src/InstanaPropagator.ts
...lugin-react-load/src/BaseOpenTelemetryComponent.ts
... and 10 more

[ryan-codaio]

@henrinormak Thanks so much for your comments. It's my first time working with the opentelemetry code, and your comments were very helpful.

[rauno56]

Other instrumentations call this feature requireParentSpan - see dataloader, ioredis or redis for example. If I'm not mistaken, and it is the same feature, we should call it with the same name for ease of use.

We often implement it slightly different as well - short-circuiting into the original function if the option is enabled and there's no parent. Not saying this implementation should be changed, but just interested in your thoughts about whether it would be viable option here and if there's any benefits going with either approach.

Thanks, @henrinormak, for the quality review. These are super helpful.

[henrinormak]

I would also prefer that naming and approach, although I don't have a strong preference, TBH, when I read the PR description, I mistakenly thought it was named like this in the referred other instrumentations as well.

[ryan-codaio]

Other instrumentations call this feature requireParentSpan - see dataloader, ioredis or redis for example. If I'm not mistaken, and it is the same feature, we should call it with the same name for ease of use.

I'm a little confused -- it's called requireParentSpan here as well, right? Not sure if I mistyped something somewhere

We often implement it slightly different as well - short-circuiting into the original function if the option is enabled and there's no parent. Not saying this implementation should be changed, but just interested in your thoughts about whether it would be viable option here and if there's any benefits going with either approach.

I'm new to opentelemetry, but my understanding is that the approach I used here (copied from instrumentation-http) is creating a placeholder span with an invalid context so that it is eventually discarded. However, child spans can be attached to this placeholder span. That way, if there are some child spans generated as a result of some pg operation when there was no parent span present, they are all discarded. This seems preferable to having some child spans show up for internal operations (e.g. DNS lookups) when we are trying to suppress pg spans with no parent span. However, I might be wrong here, so please correct me if that doesn't seem right.

[vmarchaud]

here as well, right? Not sure if I mistyped something somewhere

I think it was because you named the PR add requiredParentTrace (which i renamed afterwards)

However, child spans can be attached to this placeholder span. That way, if there are some child spans generated as a result of some pg operation when there was no parent span present, they are all discarded

I believe Rauno was refering to the usage of the non-recording span, which is used in the http instrumentation to still propagate context for outgoing requests without creating a span locally, i'm not sure we need this as we don't propagate the context to pg itself but i agree that it doesn't hurt anything to do so