fix(instrumentation-grpc): instrument @grpc/grpc-js Client methods

[pichlermarc]

Which problem is this PR solving?

Traces when using the @openfeature/flagd-provider are not connected as the @grpc/grpc-js Client methods used by @protobuf-ts/grpc-transport are not instrumented, (see instance creation, makeUnaryRequest, makeServerStreamRequest, makeClientStreamRequest, makeBidiStreamRequest) .

This was presumably not added initially as the Client class is only used via its prototype in clients generated by makeGenericClientConstructor()/makeClientConstructor(), which means that patching the methods on the Client class would not cause the patched method ever to be used.

Because clients generated by makeGenericClientConstructor()/makeClientConstructor() do not use the patched methods, it is possible to add instrumentation to those methods and not affect telemetry generated by the existing instrumentation code. This PR patches these un-instrumented client methods to provide context propagation and telemetry generation when the Client is used directly.

Fixes #3775

Short description of the changes

Adds instrumentation for the client methods

• makeUnaryRequest
• makeClientStreamRequest
• makeServerStreamRequest
• makeBidiStreamRequest

Type of change

I'm unsure whether to categorize this as a bugfix or a new feature. The Client class in @grpc/grpc-js is public, therefore I think it is entirely possible that people would use the class directly, as is the case in @protobuf-ts/grpc-transport

• Bug fix (non-breaking change which fixes an issue)
• New feature? (non-breaking change which adds functionality)

How Has This Been Tested?

• Added test cases based on the existing client tests for @grpc/grpc-js
• Manual testing using the @openfeature/flagd-provider

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

Merging #3804 (0753de5) into main (1a7488e) will increase coverage by 0.03%.
The diff coverage is 92.72%.

❗ Current head 0753de5 differs from pull request most recent head 6ae9ec9. Consider uploading reports for the commit 6ae9ec9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3804      +/-   ##
==========================================
+ Coverage   92.31%   92.35%   +0.03%     
==========================================
  Files         321      321              
  Lines        9189     9249      +60     
  Branches     1953     1962       +9     
==========================================
+ Hits         8483     8542      +59     
- Misses        706      707       +1     

Files Changed	Coverage Δ
...elemetry-instrumentation-grpc/src/grpc-js/index.ts	92.04% <91.52%> (-0.64%)	⬇️
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts	96.70% <94.11%> (+3.84%)	⬆️

... and 1 file with indirect coverage changes

[darren-west]

Anything blocking getting this merged? Ive run into this issue also

[pichlermarc]

Anything blocking getting this merged? Ive run into this issue also

Currently, this is just missing reviews from @open-telemetry/javascript-approvers - we'll need to get at least one green check to merge, though I'd like to have at least two as this is quite a significant change. I don't know if the original authors of this instrumentation are still around, which further complicates the process.

[Flarna]

left a few comments but as I'm not an grpc expert may review doesn't cover all these inner grpc details.

[pichlermarc]

Sorry for the force-push - I must've messed up my history somewhere and there were too many commits to figure out where it went wrong. 😞

[pichlermarc]

@Flarna, thank you for the review; it has been extremely helpful as I'm not as familiar with writing instrumentations as I'd like to be - I made some quite substantial changes since the last review based on your suggestions. 🙂

I added a few questions to your comments - mostly regarding modifying the pre-existing code that I moved to helper functions.

[dyladan]

Really large change and I'm also not a grpc expert but it looks ok to me

[beeme1mr]

Hey @pichlermarc, would it be possible to merge this soon?

[pichlermarc]

Hey @pichlermarc, would it be possible to merge this soon?

Yes, sorry @beeme1mr, I was out for a while and did not notice we already have enough approvals now. Failing EOL tests are unrelated (see #4030)

[pichlermarc]

/easycla

Edit: looks like easycla is broken right now. communitybridge/easycla#4071, we'll have to wait until it's back.

[mydea]

I think this lead to #4053 (but not sure, just digging around a bit)!