fix(pg): update requireParentSpan to skip instrumentation when parent not present

[ryan-codaio]

Which problem is this PR solving?

I previously added a requireParentSpan config flag that can be used to suppress instrumentation if there is no active trace (see #1199). The implementation created a no-op span, imitating the implementation for the requireParent flags in instrumentation-http, then continued running instrumentation code as usual.

This doesn't seem to be working well in practice. There are cases where the pg instrumentation creates a (no-op) span, then does something else that tries to create another span. The second invocation sees that a parent span exists, so it creates a real span. But this span is created as a sibling of the no-op span, not as a child, so then it gets emitted as a real trace. For example, I saw cases where BoundPool.connect created a (no-op) span and then called Client.connect, which saw an existing span in the context and subsequently created a (real) span.

I considered modifying startSpan to create real spans as children of whatever currentSpan was returned by trace.getSpan(context.active()); that way, the spans would get dropped as children of a no-op span. But it doesn't look like other opentelemetry instrumentation libraries do this often. I'm not very familiar with opentelemetry so I'm not sure if this is the right thing to do. It seems like a context can have multiple sibling spans, so it seems like making the span a child of trace.getSpan(context.active()) could end up creating a span as a child of the wrong parent span?

Short description of the changes

I changed the requireParentSpan implementation to avoid running instrumentation on calls when a parent span is required but doesn't exist. This way, no span is created at all. This matches the implementation in instrumentation-ioredis.

[codecov]

Codecov Report

Merging #1343 (deb0cf9) into main (52136d8) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
- Coverage   96.08%   95.82%   -0.26%     
==========================================
  Files          14       18       +4     
  Lines         893     1221     +328     
  Branches      191      263      +72     
==========================================
+ Hits          858     1170     +312     
- Misses         35       51      +16     

Impacted Files	Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts	91.21% <100.00%> (ø)
...node/opentelemetry-instrumentation-pg/src/utils.ts	98.31% <100.00%> (ø)
...try-instrumentation-pg/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	98.21% <0.00%> (ø)

[ryan-codaio]

Hi, would I be able to get a review on this PR? Thanks in advance!

[ryan-codaio]

Hi @dyladan, sorry for the ping but it seems like @rauno56 may unavailable, and it looks like you're the most active maintainer. Do you know of someone else who might be able to review this PR? It's been sitting for a month and I'm hoping to be able to get it merged 🙏

[blumamir]

LGTM

sorry about the time it took to review

[ryan-codaio]

Thank you so much for the review!