feat(plugin): add grpc plugin #215

markwolff · 2019-08-22T22:11:20Z

Which problem is this PR solving?

Add gRPC plugin #158 - Add gRPC plugin

Short description of the changes

Adds trace collection plugin for gRPC

TODOs

Determine what to do with attributes. They were in the OC impl, but not in any grpc spec I've seen.
https://github.com/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/opencensus/gRPC-stats.md
rebase for TODOS in feat(plugin): add http plugin #161 (ProxyTracer, etc)
rebase for PluginConfig

OlivierAlbertini

Look good. Added few comments. methods and properties (private and protected) should be prefixed with "_"

OlivierAlbertini · 2019-08-22T22:24:37Z

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

+
+export class GrpcPlugin extends BasePlugin<grpc> {
+  static readonly component = 'grpc';
+  static readonly ATTRIBUTE_GRPC_KIND = 'grpc.kind'; // SERVER or CLIENT


Should move to AttributeNames, WDYT?

OlivierAlbertini · 2019-08-22T22:25:14Z

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

+  // TODO: Delete if moving internal file loaders to BasePlugin
+  // --- Note: Incorrectly ordered: Begin internal file loader --- //
+  // tslint:disable-next-line:no-any
+  protected internalFilesExports: { [key: string]: any };


Should be prefixed by _

markwolff · 2019-08-22T22:32:21Z

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

+    // TODO: Remove this once issue is resolved
+    // https://github.com/open-telemetry/opentelemetry-js/issues/193
+    this._logger = new NoopLogger();
+    this.internalFilesExports = this.loadInternalFiles();


this can be removed pending discussion #216

mayurkale22

This is a great start :) Added a few comments, during initial glance. Will add more once ready for review.

packages/opentelemetry-plugin-grpc/package.json

packages/opentelemetry-plugin-grpc/src/enums/attributeNames.ts

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

draffensperger · 2019-08-31T16:44:51Z

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

@@ -247,29 +237,23 @@ export class GrpcPlugin extends BasePlugin<grpc> {
              const self = this;

              const spanName = `grpc.${name.replace('/', '')}`;
+              const parentSpan = plugin._getSpanContext(call.metadata);


Is this calling a private function on the plugin? If so, why does it need to do that?

It only needs it because it needs the binary format parser which is a member of this._tracer. I suppose it could be made a helper function similar to your other proposal, but a tracer (or the binary format parser directly) would need to be passed in instead. What do you think?

draffensperger · 2019-08-31T16:46:06Z

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

+   * Convert a grpc status code to an opentelemetry Canonical code. For now, the enums are exactly the same
+   * @param status
+   */
+  private static _grpcStatusCodeToCanonicalCode(


Rather than making this private static, what would you think about just having it be a non-exported function in the file? I personally find that cleaner for helper functions than attaching them to a class

I have an existing utils.ts already. Maybe I can just move them inside there?

mayurkale22 · 2019-09-04T21:59:26Z

packages/opentelemetry-plugin-grpc/package.json

@@ -6,7 +6,7 @@
  "types": "build/src/index.d.ts",
  "repository": "open-telemetry/opentelemetry-js",
  "scripts": {
-    "test": "nyc ts-mocha -p tsconfig.json test/**/*.ts",
+    "test": "nyc ts-mocha -p tsconfig.json test/**/*.test.ts",


I'd suggest to keep it standardized everywhere, either change all packages script to test/**/*.test.ts or keep it test/**/*.ts. WDYT?

packages/opentelemetry-plugin-grpc/package.json

packages/opentelemetry-plugin-grpc/src/enums/grpcAttributeNames.ts

packages/opentelemetry-plugin-grpc/src/grpc-types.ts

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

mayurkale22 · 2019-09-04T22:16:30Z

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

+   * Convert a grpc status code to an opentelemetry Canonical code. For now, the enums are exactly the same
+   * @param status
+   */
+  private static _grpcStatusCodeToCanonicalCode(


packages/opentelemetry-plugin-grpc/test/utils/SpanAuditProcessor.ts

draffensperger · 2019-09-06T13:41:38Z

packages/opentelemetry-plugin-grpc/src/types.ts

+  metadata: grpcModule.Metadata;
+  status: GrpcStatus;
+  // tslint:disable-next-line:no-any
+  request?: any;


Would it work for this to be of unknown type?

Changed to unknown type (and a few other places where it made sense)

draffensperger · 2019-09-06T13:42:44Z

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

+ * limitations under the License.
+ */
+
+// Equivalent to lodash _.findIndex


Thanks for writing this utility rather than pulling in lodash itself! (I have seen vulnerabilities on it recently and wouldn't want to expose OpenTelemetry customers to that risk)

mayurkale22 · 2019-09-09T19:21:15Z

LGTM. @OlivierAlbertini @bg451 @danielkhan Could you please approve this one, if everything looks good, Thanks :)

markwolff · 2019-09-09T19:26:13Z

A note about this and #216, I intend to complete this PR and then stub out the internalFilesExports functionality in a separate PR which will also rip out that functionality from this plugin (https://github.com/open-telemetry/opentelemetry-js/pull/215/files#diff-671cbf0fed27b96717e412e39a4dfc30R65). I have no other changes pending for this PR (other than parsing PluginConfig), so if it looks good to everyone else, it can be completed.

bg451 · 2019-09-11T15:42:26Z

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

+
+              plugin._logger.debug(
+                'patch func: %s',
+                JSON.stringify(spanOptions)


out of curiosity/ignorance, will this json.stringify be executed when logging is not DEBUG?

Yes, it would be executed and then passed to a noop function. Seems wasteful so I'll put this (and anything similar I find) behind an if.

edit: AFAIK the current logging "level" is not implemented/exposed anywhere? If so, I will make this change in a separate PR.

LogLevel is already implemented and exposed here :

opentelemetry-js/packages/opentelemetry-basic-tracer/src/types.ts

Line 53 in 52a86c9

logLevel?: LogLevel;

This will be called even if the log level is not matched. This is an expensive call, let's address this in a separate PR.

markwolff added 3 commits August 22, 2019 15:06

feat: add grpc plugin

0db3a5b

fix: remove test typo

52d5024

fix: nyc missing from test

92ef3c5

OlivierAlbertini reviewed Aug 22, 2019

View reviewed changes

markwolff commented Aug 22, 2019

View reviewed changes

fix: linting and NodeTracer export

5382516

mayurkale22 reviewed Aug 23, 2019

View reviewed changes

markwolff added 3 commits August 28, 2019 18:23

chore: rename grpc test file

dfb24f0

test: add error code tests

08e0fba

Merge branch 'master' into add-grpc-plugin

052dc53

markwolff marked this pull request as ready for review August 30, 2019 20:54

markwolff requested review from danielkhan, iredelmeier and rochdev as code owners August 30, 2019 20:54

refactor: logger is given to plugin

02c1d4c

markwolff requested a review from bg451 as a code owner August 30, 2019 21:48

mayurkale22 mentioned this pull request Aug 30, 2019

Nominating @OlivierAlbertini as JavaScript approver open-telemetry/community#166

Merged

markwolff added 2 commits August 30, 2019 15:34

Merge branch 'master' into add-grpc-plugin

91d8a25

fix(grpc): using logger before defined

64a805d

draffensperger reviewed Aug 31, 2019

View reviewed changes

mayurkale22 reviewed Sep 4, 2019

View reviewed changes

markwolff added 3 commits September 4, 2019 16:46

refactor: move private statics to internal file

aefc3e4

test: add withSpan OK test

f0bac7b

refactor: follow plugin naming pattern

2d01b61

mayurkale22 reviewed Sep 5, 2019

View reviewed changes

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

mayurkale22 reviewed Sep 5, 2019

View reviewed changes

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

markwolff added 2 commits September 5, 2019 14:13

Merge branch 'master' into add-grpc-plugin

e9793a4

refactor(tests): replace ProxyTracer

8633a7f

draffensperger approved these changes Sep 6, 2019

View reviewed changes

mayurkale22 approved these changes Sep 6, 2019

View reviewed changes

markwolff added 3 commits September 9, 2019 13:51

refactor(grpc): move helper funcs to utils

798bc4b

Merge branch 'master' into add-grpc-plugin

51bb59c

refactor(tests): use new memory exporter helper

df5aed8

danielkhan approved these changes Sep 10, 2019

View reviewed changes

bg451 approved these changes Sep 11, 2019

View reviewed changes

Merge branch 'master' into add-grpc-plugin

4eed3e8

mayurkale22 merged commit c675c86 into open-telemetry:master Sep 11, 2019

mayurkale22 mentioned this pull request Sep 11, 2019

Add gRPC plugin #158

Closed

markwolff deleted the add-grpc-plugin branch September 12, 2019 12:41

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

Allow redis version 3.0.0 and above (open-telemetry#215)

807346b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin): add grpc plugin #215

feat(plugin): add grpc plugin #215

markwolff commented Aug 22, 2019 •

edited

Loading

OlivierAlbertini left a comment

OlivierAlbertini Aug 22, 2019

OlivierAlbertini Aug 22, 2019

markwolff Aug 22, 2019

mayurkale22 left a comment

draffensperger Aug 31, 2019

markwolff Sep 4, 2019

draffensperger Aug 31, 2019

mayurkale22 Sep 4, 2019

markwolff Sep 4, 2019

mayurkale22 Sep 4, 2019

mayurkale22 Sep 4, 2019

draffensperger Sep 6, 2019

markwolff Sep 9, 2019

draffensperger Sep 6, 2019

mayurkale22 commented Sep 9, 2019

markwolff commented Sep 9, 2019 •

edited

Loading

bg451 Sep 11, 2019

markwolff Sep 11, 2019 •

edited

Loading

mayurkale22 Sep 11, 2019

mayurkale22 Sep 11, 2019

feat(plugin): add grpc plugin #215

feat(plugin): add grpc plugin #215

Conversation

markwolff commented Aug 22, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

TODOs

OlivierAlbertini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Sep 9, 2019

markwolff commented Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

markwolff Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwolff commented Aug 22, 2019 •

edited

Loading

markwolff commented Sep 9, 2019 •

edited

Loading

markwolff Sep 11, 2019 •

edited

Loading