feat(grpc-js): add @grpc/grpc-js plugin #1201

markwolff · 2020-06-16T17:52:55Z

Which problem is this PR solving?

Closes Add support for @grpc/grpc-js 1.x #1135

Short description of the changes

Not a lot of "new" code here. Most of it is copied or moved over from the existing grpc plugin.

Adds patching for @grpc/grpc-js library
Creates new @opentelemetry/grpc-utils private package which contains shared test suite that will be used by @opentelemetry/plugin-grpc / @opentelemetry/plugin-grpc-js. This code is what was previously in grpc.test.ts.
- I do not think it is useful longterm to share patching here since there are a few subtle internal differences between grpc and @grpc/grpc-js and they could further diverge in the future.
- Opentelemetry "span management" could be shared, however! (e.g. attributes, statuses, links, etc)

- opentelemetry-plugin-grpc-js
- - opentelemetry-grpc-utils/test
- opentelemetry-plugin-grpc
- - opentelemetry-grpc-utils/test

…nto feat/add-grpc-js-plugin

codecov · 2020-06-16T17:59:04Z

Codecov Report

Merging #1201 into master will increase coverage by 0.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
+ Coverage   92.37%   92.69%   +0.32%     
==========================================
  Files         181      193      +12     
  Lines        4378     4804     +426     
  Branches      922      982      +60     
==========================================
+ Hits         4044     4453     +409     
- Misses        334      351      +17

Impacted Files	Coverage Δ
...ry-plugin-xml-http-request/src/enums/EventNames.ts	`100.00% <0.00%> (ø)`
...ackages/opentelemetry-plugin-grpc-js/src/grpcJs.ts	`100.00% <0.00%> (ø)`
...packages/opentelemetry-plugin-grpc-js/src/utils.ts	`88.88% <0.00%> (ø)`
...s/opentelemetry-plugin-grpc-js/src/client/utils.ts	`96.59% <0.00%> (ø)`
...y-plugin-grpc-js/src/server/serverStreamAndBidi.ts	`100.00% <0.00%> (ø)`
...telemetry-plugin-grpc-js/src/server/patchServer.ts	`95.00% <0.00%> (ø)`
...-plugin-grpc-js/src/server/clientStreamAndUnary.ts	`100.00% <0.00%> (ø)`
...s/opentelemetry-plugin-grpc-js/src/server/index.ts	`100.00% <0.00%> (ø)`
...s/opentelemetry-plugin-grpc-js/src/client/index.ts	`100.00% <0.00%> (ø)`
...plugin-grpc-js/src/client/loadPackageDefinition.ts	`100.00% <0.00%> (ø)`
... and 2 more

obecny

just first pass as the pr is big

packages/opentelemetry-plugin-grpc-js/src/grpcJs.ts

packages/opentelemetry-plugin-grpc-js/src/server/clientStreamAndUnary.ts

packages/opentelemetry-plugin-grpc-js/src/server/patchServer.ts

packages/opentelemetry-plugin-grpc-js/src/client/patchClient.ts

obecny · 2020-06-16T19:05:44Z

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts

+      .map(methodName => methods[methodName].originalName)
+      .filter(
+        originalName =>
+          // eslint-disable-next-line no-prototype-builtins


should we disable this rule completely ?

Hm, do you mean for the entire project? I am not opposed... I think the only benefit it has for this project is to gently discourage using builtins unless necessary

we are patching many things, so I guess this might be not needed rule at all for this project, this is just my thought :)

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts

obecny · 2020-06-16T19:15:00Z

what's happened to unit tests ?

markwolff · 2020-06-16T21:25:36Z

what's happened to unit tests ?

The API layers between grpc & @grpc/grpc-js are the same and so I've moved the tests to a shared & unpublished @opentelemetry/grpc-utils package.

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts

packages/opentelemetry-plugin-grpc-js/src/utils.ts

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts

…nto feat/add-grpc-js-plugin

obecny

have 2 questions yet, but it lgtm

packages/opentelemetry-plugin-grpc-js/src/grpcJs.ts

obecny · 2020-06-29T12:12:25Z

packages/opentelemetry-plugin-grpc-js/src/grpcJs.ts

    super('@opentelemetry/plugin-grpc-js', VERSION);
  }

  protected patch(): typeof grpcJs {
-    throw new Error('Method not implemented.');
+    this.tracer = this._tracer;


should this be somehow unpatched in unpatch

Good point. This would be to prevent a dangling reference on this reference right? I've added jsdoc comment to their declarations and nulled them out here via

(this.tracer as Tracer | null) = null; (this.logger ...) = null

I don't think a dangling reference here is a big deal as there can only ever be one of them. If there was a new reference on each call to patch then maybe it would be a problem.

…nto feat/add-grpc-js-plugin

mayurkale22 · 2020-07-14T04:58:44Z

@open-telemetry/javascript-approvers ^^

sk-

Added suggestions around slices for copying an object, which we can change to use the spread operator instead, which has been supported since node 5. See https://node.green/#ES2015-syntax-spread-syntax-for-iterable-objects

packages/opentelemetry-grpc-utils/test/grpcUtils.test.ts

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts

packages/opentelemetry-grpc-utils/test/grpcUtils.test.ts

vmarchaud

lgtm

vmarchaud · 2020-07-18T07:32:15Z

packages/opentelemetry-plugin-grpc-js/src/server/serverStreamAndBidi.ts

+): void {
+  let spanEnded = false;
+  const endSpan = () => {
+    if (!spanEnded) {


I'm thinking that we should expose a property on the SDK's span that represent if the span has been ended. There are few plugins that implement something like this which add memory overhead for nothing, WDYT ?

…nto feat/add-grpc-js-plugin

packages/opentelemetry-grpc-utils/test/grpcUtils.test.ts

dyladan · 2020-07-21T14:07:50Z

@sk- thanks for your thorough review :)

@dyladan

… becoming an owner (open-telemetry#1201) * chore(instrumentation-graphql): turn off failing test and releases * chore: keep releasing GraphQL * chore: fix wording on readme * chore: apply @dyladan's suggestions Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

markwolff added 3 commits June 15, 2020 11:52

feat(grpc): implement grpc-js plugin

2014f11

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

d9b5017

…nto feat/add-grpc-js-plugin

feat(grpc-js): add plugin-grpc-js impl

2363e18

markwolff requested review from bg451, dyladan, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners June 16, 2020 17:52

markwolff changed the title ~~Feat/add grpc js plugin~~ feat(grpc-js): add @grpc/grpc-js plugin Jun 16, 2020

obecny reviewed Jun 16, 2020

View reviewed changes

fix: fix conflicting tests, add jsdoc

ac447f1

sk- reviewed Jun 20, 2020

View reviewed changes

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts Outdated Show resolved Hide resolved

sk- reviewed Jun 20, 2020

View reviewed changes

packages/opentelemetry-plugin-grpc-js/src/utils.ts Outdated Show resolved Hide resolved

sk- reviewed Jun 20, 2020

View reviewed changes

packages/opentelemetry-plugin-grpc-js/src/client/utils.ts Outdated Show resolved Hide resolved

markwolff added 2 commits June 22, 2020 11:59

refactor: remove findIndex

ef26169

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

0632de3

…nto feat/add-grpc-js-plugin

markwolff requested a review from legendecas as a code owner June 22, 2020 16:02

markwolff added 5 commits June 22, 2020 12:26

fix: build errors

b20ecdd

refactor: use builtin version

7e8734d

style(docs): add missing jsdocs

495a18f

fix: address remaining comments

9b76fdb

fix: add correct jsdocs

b22631e

obecny approved these changes Jun 29, 2020

View reviewed changes

markwolff added 4 commits June 29, 2020 14:17

refactor: null public plugin refs on unpatch

8c24f68

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

66a5649

…nto feat/add-grpc-js-plugin

style: fix linting

d1c3f2d

refactor: use get tracer()

b7b2c93

sk- reviewed Jul 15, 2020

View reviewed changes

dyladan approved these changes Jul 17, 2020

View reviewed changes

vmarchaud approved these changes Jul 18, 2020

View reviewed changes

mayurkale22 approved these changes Jul 18, 2020

View reviewed changes

markwolff added 3 commits July 20, 2020 11:34

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

16ee1dd

…nto feat/add-grpc-js-plugin

fix(grpc-js): add missing unit tests

5496343

refactor: address pending comments

4daf978

sk- reviewed Jul 20, 2020

View reviewed changes

packages/opentelemetry-grpc-utils/test/grpcUtils.test.ts Outdated Show resolved Hide resolved

packages/opentelemetry-grpc-utils/test/grpcUtils.test.ts Outdated Show resolved Hide resolved

refactor: rm redundant array copy

9a6df70

sk- approved these changes Jul 20, 2020

View reviewed changes

Merge branch 'master' into feat/add-grpc-js-plugin

5bfc33f

mayurkale22 merged commit e056558 into open-telemetry:master Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(grpc-js): add @grpc/grpc-js plugin #1201

feat(grpc-js): add @grpc/grpc-js plugin #1201

markwolff commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

obecny left a comment

obecny Jun 16, 2020

markwolff Jun 22, 2020

obecny Jun 22, 2020

obecny commented Jun 16, 2020

markwolff commented Jun 16, 2020

obecny left a comment •

edited

Loading

obecny Jun 29, 2020

markwolff Jun 29, 2020

dyladan Jun 29, 2020

mayurkale22 commented Jul 14, 2020

sk- left a comment

vmarchaud left a comment

vmarchaud Jul 18, 2020

dyladan commented Jul 21, 2020

feat(grpc-js): add @grpc/grpc-js plugin #1201

feat(grpc-js): add @grpc/grpc-js plugin #1201

Conversation

markwolff commented Jun 16, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 16, 2020 • edited Loading

Codecov Report

obecny left a comment

Choose a reason for hiding this comment

obecny Jun 16, 2020

Choose a reason for hiding this comment

markwolff Jun 22, 2020

Choose a reason for hiding this comment

obecny Jun 22, 2020

Choose a reason for hiding this comment

obecny commented Jun 16, 2020

markwolff commented Jun 16, 2020

obecny left a comment • edited Loading

Choose a reason for hiding this comment

obecny Jun 29, 2020

Choose a reason for hiding this comment

markwolff Jun 29, 2020

Choose a reason for hiding this comment

dyladan Jun 29, 2020

Choose a reason for hiding this comment

mayurkale22 commented Jul 14, 2020

sk- left a comment

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

vmarchaud Jul 18, 2020

Choose a reason for hiding this comment

dyladan commented Jul 21, 2020

markwolff commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

obecny left a comment •

edited

Loading