gRPC plugin is not working correctly when loaded with @grpc/proto-loader

[mayurkale22]

When a proto file is loaded with @grpc/proto-loader, gRPC plugin instrumentation is not working correctly. Client and Server traces has different traceIds.

Client Traces:

Server Traces:

May be something to do with the binary propagation or client methods are not patched properly.

[mayurkale22]

/cc @markwolff

[markwolff]

Haven't looked into this yet, but it's probably for a similar reason that patching the internal function makeClientConstructor didn't also patch the externally re-exported makeGenericClientConstructor. Did this scenario work in OC? The issue could be that internal files are patched after external files are.

[mayurkale22]

Did this scenario work in OC?

No, it is not working as expected.

[mayurkale22]

Strangly, with OC when I used lowercase method name in protos: https://github.com/open-telemetry/opentelemetry-js/pull/385/files#diff-a10e9a452e0c1da7f8f0d0f5a70622e4R12, everything works well. Maybe, we are not patching all the client methods properly: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-grpc/src/grpc.ts#L336-L341

[markwolff]

Strangly, with OC when I used lowercase method name in protos: https://github.com/open-telemetry/opentelemetry-js/pull/385/files#diff-a10e9a452e0c1da7f8f0d0f5a70622e4R12, everything works well. Maybe, we are not patching all the client methods properly: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-grpc/src/grpc.ts#L336-L341

Yes the lowercase stuff was a bug I found in OC as well which would be fixed by this. If the lowercase name works in OC, then I shouldn't have to use { internals: true } and there is a bug with the existing patching.

[mayurkale22]

there is a bug with the existing patching.

Possible. While testing found out https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-grpc/src/grpc.ts#L328 is not getting called when using proto-loader.

[markwolff]

Looked into this, it is because of lerna linking. The grpc-plugin's devdep grpc internal files are patched because it grabs the basedir within the plugin. For a production sample, I'd imagine it would work because the dev dependency won't be installed and so basedir would resolve to the correct path. To compare with OC, basedir was set in plugin-loader.

I see 2 ways of resolving this:

1. Ignore it since it would work in an example outside of this repo. Maybe remove lerna linking from this example or add some hacks to how basedir is resolved.
2. Update the plugin.enable(...) signature to include basedir/version information as it did in OpenCensus

Either way, I will open a PR to fix the issue with .capitalize(...) vs .Capitalize(...)

@mayurkale22 WDYT?

[mayurkale22]

I am fine with the option 1. Do you think I should merge the example - gRPC example using proto-loader #385?

[markwolff]

I think thats fine. It just wont work if using it by cloning this whole repo.

[mayurkale22]

Closing via #631