grpc->grpc-js #1778

dradetsky · 2020-12-23T01:48:08Z

NOTE: this is a draft just so it's easier to have a discussion about the issue. I know I'm not doing the commits properly.

Which problem is this PR solving?

opentelemetry-js uses grpc, even though this has been deprecated in favor of grpc-js
i'm personally seeing build times for my services instrumented with otel going WAY up as we compile all this C++ (eww). this is bad enough now, and going to get worse as we apply otel instrumentation to all our other microservices.
previously , we wanted to upgrade to nodejs 15.x.x, but ran into bugs with node-gyp during build & couldn't do so. I know technically you guys don't concern yourself with non-lts node versions, and we were able to fix this by using node 14.x.x which is fine for us, but wouldn't you rather not have these kinds of dependency issues?

Short description of the changes

replacing grpc with grpc-js
modifying the tests so they still work with grpc-js (this is where the difficulty comes in)

As you can see, despite the claims, grpc-js is not really a drop-in replacement for grpc. I'm sure the client is a drop-in replacement, but the server isn't, and I don't know enough about this test framework & how the server is supposed to work normally to fix it.

For reference: https://www.npmjs.com/package/@grpc/grpc-js

linux-foundation-easycla · 2020-12-23T01:48:11Z

The committers are authorized under a signed CLA.

✅ dmr (a08e865, 61106d3)

codecov · 2020-12-23T01:57:04Z

Codecov Report

Merging #1778 (7bbed80) into master (a2304c9) will increase coverage by 0.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1778      +/-   ##
==========================================
+ Coverage   92.11%   92.52%   +0.40%     
==========================================
  Files         166       69      -97     
  Lines        5571     1913    -3658     
  Branches     1197      409     -788     
==========================================
- Hits         5132     1770    -3362     
+ Misses        439      143     -296

Impacted Files	Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts	`0.00% <0.00%> (-100.00%)`	⬇️
.../opentelemetry-api/src/platform/node/globalThis.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...pentelemetry-core/src/platform/node/performance.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`80.85% <0.00%> (-19.15%)`	⬇️
...ckages/opentelemetry-core/src/utils/environment.ts	`87.23% <0.00%> (ø)`
...kages/opentelemetry-exporter-collector/src/util.ts	`100.00% <0.00%> (ø)`
...elemetry-core/src/context/propagation/composite.ts	`100.00% <0.00%> (ø)`
...y-plugin-grpc-js/src/server/serverStreamAndBidi.ts
packages/opentelemetry-context-base/src/context.ts
...detector-aws/src/detectors/AwsBeanstalkDetector.ts
... and 95 more

dyladan · 2020-12-23T14:59:21Z

Please also make sure you sign the CLA or we cannot accept your contribution.

dradetsky · 2020-12-23T16:52:34Z

@dyladan as I say, I didn't particularly expect this version to be accepted since I didn't bother doing it right (i used --no-verify). But I'll sign the CLA just in case.

dyladan · 2020-12-23T16:54:40Z

I started looking into this error and it's a tricky one. The failure occurs somewhere deep in the internals of grpc-js and it's hard to see what's happening

dradetsky · 2020-12-23T19:12:55Z

@dyladan assuming you're referring to

Uncaught TypeError: Cannot read property 'interceptors' of undefined

Is there reason to believe this is actually a problem with the grpc-js client & thus that it's actually not compatible? Or is it just a test problem?

dyladan · 2020-12-23T19:13:53Z

@dyladan assuming you're referring to
Uncaught TypeError: Cannot read property 'interceptors' of undefined
Is there reason to believe this is actually a problem with the grpc-js client & thus that it's actually not compatible? Or is it just a test problem?

Yes that's my feeling. I considered that it might be a bug with the client, but in order to prove that i would want to create a much more minimal reproduction and I don't really have time for that at the moment.

dradetsky · 2020-12-23T19:59:12Z

@dyladan fortunately, I have time. It's amazing how efficiently you can work when you remove the requirement of "not writing code that is absolute, utter shit."

anyway, pulled what i think is the equivalent of what we do in the test out into just a normal script and it works fine. I even got into it with the debugger (which I had originally tried to set up to debug inside of mocha) and the callProperties.callOptions looked fine (it was {}, but at least you could read its .interceptors).

That's the annoying part btw; I know how to set a breakpoint in grpc-js's client.ts and run a jest test with the debugger so i break in the test, but doing the same steps with mocha failed. And I definitely don't have time to learn how to do weird things with mocha.

dyladan · 2020-12-23T20:01:42Z

@dyladan fortunately, I have time. It's amazing how efficiently you can work when you remove the requirement of "not writing code that is absolute, utter shit."

anyway, pulled what i think is the equivalent of what we do in the test out into just a normal script and it works fine. I even got into it with the debugger (which I had originally tried to set up to debug inside of mocha) and the callProperties.callOptions looked fine (it was {}, but at least you could read its .interceptors).

That's the annoying part btw; I know how to set a breakpoint in grpc-js's client.ts and run a jest test with the debugger so i break in the test, but doing the same steps with mocha failed. And I definitely don't have time to learn how to do weird things with mocha.

You can run tests with debugging enabled by doing npm test -- --inspect-brk and connecting to it with vs code's "Attach to process" command. I typically also do npm test -- -b --inspect-brk so the tests will stop running when a failure is encountered.

You can also run only a single test file by running the test command manually npx nyc ts-mocha -p tsconfig.json -b --inspect-brk test/path/to/my.test.ts

dradetsky · 2020-12-23T20:13:47Z

So it turns out that the problem was that the options object was actually secretly a function returning

CollectorExporterNodeBase.ts? [sm]:68 Uncaught ReferenceError: Cannot access 'promise' before initialization
    at _onFinish (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:1158)
    at _onSuccess (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:820)
    at Object.options (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4)
    at eval (eval at makeUnaryRequest (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/client.ts:292:5), <anonymous>:1:1)
    at ServiceClientImpl.makeUnaryRequest (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/client.ts:292:5)
    at ServiceClientImpl.<anonymous> (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/make-client.ts:177:15)
    at CollectorTraceExporter.send [as _send] (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4)
    at promise (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:1263)
    at new Promise (<anonymous>)
    at CollectorTraceExporter._sendPromise (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:641)
    at CollectorTraceExporter.send (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:45:55)
    at /home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4
    at Array.forEach (<anonymous>)
    at /home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4

which was subsequently converted by checkOptionalUnaryResponseArguments<ResponseType> to undefined (this is probably a bad thing for it to do).

In any case, my suspicion all along was that something in how the async code was done in this test is causing strange things to happen.

dyladan · 2020-12-23T20:14:33Z

So it turns out that the problem was that the options object was actually secretly a function returning

CollectorExporterNodeBase.ts? [sm]:68 Uncaught ReferenceError: Cannot access 'promise' before initialization
    at _onFinish (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:1158)
    at _onSuccess (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:820)
    at Object.options (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4)
    at eval (eval at makeUnaryRequest (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/client.ts:292:5), <anonymous>:1:1)
    at ServiceClientImpl.makeUnaryRequest (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/client.ts:292:5)
    at ServiceClientImpl.<anonymous> (/home/dmr/job/repo/bukkitz/vendor/grpc-node/packages/grpc-js/src/make-client.ts:177:15)
    at CollectorTraceExporter.send [as _send] (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4)
    at promise (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:1263)
    at new Promise (<anonymous>)
    at CollectorTraceExporter._sendPromise (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:18:641)
    at CollectorTraceExporter.send (/home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts:45:55)
    at /home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4
    at Array.forEach (<anonymous>)
    at /home/dmr/job/repo/bukkitz/vendor/opentelemetry-js/packages/opentelemetry-exporter-collector-grpc/src/util.ts:15:4

which was subsequently converted by checkOptionalUnaryResponseArguments<ResponseType> to undefined (this is probably a bad thing for it to do).

In any case, my suspicion all along was that something in how the async code was done in this test is causing strange things to happen.

Does that mean you discovered a solution?

dradetsky · 2020-12-23T20:19:01Z

@dyladan I do not. I mean that I believe this is a bug in the test's async code & not a sign that grpc-js is incompatible with the exporter code.

In other words, I think that if you were to rewrite the test more carefully to set up in the proper order (probably with async/await), it would work fine.

vmarchaud · 2021-04-17T20:25:38Z

Closing since this has been done with #2092

dradetsky added 2 commits December 22, 2020 17:32

switch to grpc-js

a08e865

bind->bindAsync, but this breaks test

61106d3

dradetsky requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners December 23, 2020 01:48

dradetsky marked this pull request as draft December 23, 2020 01:48

doesnt break even tho the mocha equivalent does

7bbed80

Base automatically changed from master to main January 25, 2021 19:26

vmarchaud mentioned this pull request Apr 8, 2021

Migrate exporter-collector-grpc to grpc-js #2087

Closed

obecny mentioned this pull request Apr 9, 2021

Migrate exporter-collector-grpc to grpc-js #2092

Merged

vmarchaud closed this Apr 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc->grpc-js #1778

grpc->grpc-js #1778

dradetsky commented Dec 23, 2020

linux-foundation-easycla bot commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020 •

edited

Loading

dradetsky commented Dec 23, 2020 •

edited

Loading

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

vmarchaud commented Apr 17, 2021

grpc->grpc-js #1778

grpc->grpc-js #1778

Conversation

dradetsky commented Dec 23, 2020

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Dec 23, 2020 • edited Loading

codecov bot commented Dec 23, 2020 • edited Loading

Codecov Report

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

dyladan commented Dec 23, 2020 • edited Loading

dradetsky commented Dec 23, 2020 • edited Loading

dyladan commented Dec 23, 2020

dradetsky commented Dec 23, 2020

vmarchaud commented Apr 17, 2021

linux-foundation-easycla bot commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading

dyladan commented Dec 23, 2020 •

edited

Loading

dradetsky commented Dec 23, 2020 •

edited

Loading