feat: add ability to filter grpc methods #1353

reggiemcdonald · 2020-07-27T22:34:29Z

Which problem is this PR solving?

This PR addresses plugin-grpc does not filter outgoing requests from exporter #1327

In brief, the current gRPC plugins have no ability to filter which methods are traced. This could lead to an infinite cycle if an exporter was to use gRPC. This PR addresses this problem.

Short description of the changes

The following changes were made for both plugin-grpc and plugin-grpc-js:

Outgoing requests with x-opentelemetry-outgoing-request metadata are now filtered
Added the ability to filter methods using the ignoreMethods plugin config

Testing

Unit tests
Tested @opentelemetry/plugin-grpc on a live instance exported to GCP
Tested @opentelemetry/plugin-grpc-js on a live instance exported to GCP

codecov · 2020-07-27T22:39:14Z

Codecov Report

Merging #1353 into master will decrease coverage by 0.17%.
The diff coverage is 94.05%.

@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
- Coverage   93.90%   93.72%   -0.18%     
==========================================
  Files         148      148              
  Lines        4396     4481      +85     
  Branches      895      929      +34     
==========================================
+ Hits         4128     4200      +72     
- Misses        268      281      +13

Impacted Files	Coverage Δ
...plugin-grpc-js/src/client/loadPackageDefinition.ts	`100.00% <ø> (ø)`
...telemetry-plugin-grpc-js/src/client/patchClient.ts	`100.00% <ø> (ø)`
...ackages/opentelemetry-plugin-grpc-js/src/grpcJs.ts	`100.00% <ø> (ø)`
...telemetry-plugin-grpc-js/src/server/patchServer.ts	`93.54% <90.90%> (-1.46%)`	⬇️
packages/opentelemetry-plugin-grpc/src/grpc.ts	`94.31% <92.59%> (-2.57%)`	⬇️
packages/opentelemetry-plugin-grpc-js/src/utils.ts	`93.10% <95.00%> (+4.21%)`	⬆️
packages/opentelemetry-plugin-grpc/src/utils.ts	`91.89% <95.23%> (-1.86%)`	⬇️
...s/opentelemetry-plugin-grpc-js/src/client/utils.ts	`93.40% <100.00%> (-3.19%)`	⬇️
... and 2 more

reggiemcdonald · 2020-07-27T22:48:25Z

/cc @nlehrer @danherr

markwolff

Overall LGTM. Added few comments

markwolff · 2020-07-29T14:10:22Z

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

+export const methodIsIgnored = (
+  methodName: string,
+  ignoredMethods?: string[]
+): boolean => {
+  if (!ignoredMethods) {
+    return false;
+  }
+  for (const pattern of ignoredMethods) {
+    if (new RegExp(`^${pattern}$`, 'i').test(methodName)) {
+      return true;
+    }
+  }
+  return false;
+};


Could this be made like http plugin and support both partial regex and exact string matching?

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

Lines 140 to 153 in 03981e4

export const satisfiesPattern = <T>(

constant: string,

pattern: IgnoreMatcher

): boolean => {

if (typeof pattern === 'string') {

return pattern === constant;

} else if (pattern instanceof RegExp) {

return pattern.test(constant);

} else if (typeof pattern === 'function') {

return pattern(constant);

} else {

throw new TypeError('Pattern is in unsupported datatype');

}

};

Yep! I'll just use the exact same approach as in the http plugin

markwolff · 2020-07-29T14:13:34Z

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

+/**
+ * Metadata key used to denote an outgoing opentelemetry request.
+ */
+const _otRequestHeader = 'x-opentelemetry-outgoing-request';


nit: This seems to be getting defined in a lot of separate places. @open-telemetry/javascript-approvers Should it be moved to somewhere common like /core or /tracing or /semantic-conventions?

Once I get consensus here I can make the change thats required

I think just keeping it here is fine for now, because a better solution in the form of #1344 is coming.

markwolff · 2020-07-29T14:15:08Z

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

+    return false;
+  }
+  for (const pattern of ignoredMethods) {
+    if (new RegExp(`^${pattern}$`, 'i').test(methodName)) {


Is there a benefit to newing up a regex here each time instead of doing a standard inequality check?

It's a case insensitive match which I believe it needs to be for grpc since the method names may be either lower or upper camel case. I could switch to using toLowerCase() instead

reggiemcdonald · 2020-07-29T20:35:17Z

Not sure whats up with the CI? The same commands run locally, and the thrown error doesn't look related to the changes in the last 2 commits

dyladan · 2020-07-29T20:38:08Z

Not sure whats up with the CI? The same commands run locally, and the thrown error doesn't look related to the changes in the last 2 commits

looks like npm returned a 404. I'm able to download the tarball it missed. I'll restart the build and see if that helps.

reggiemcdonald · 2020-07-29T20:44:59Z

Not sure whats up with the CI? The same commands run locally, and the thrown error doesn't look related to the changes in the last 2 commits

looks like npm returned a 404. I'm able to download the tarball it missed. I'll restart the build and see if that helps.

Looks like that solved the issue. Thanks!

obecny · 2020-08-06T15:05:12Z

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

+ * Returns true if the server call should not be traced.
+ */
+function shouldNotTraceServerCall(
+  this: GrpcJsPlugin,


can you please change this into grpcJsPlugin, I find it a bit misleading later in code

If I change this to grpcJsPlugin then I cant access the _config of the plugin because it is a protected field. Do you have any suggestions around this?

then function shouldNotTraceServerCall should not be a standalone function but a private of GrpcJsPlugin

Because the function needs to be called in patchServer(grpcJsPlugin) I cant actually make the function a private member of the class.

One alternative would be to remove the this keyword from shouldNotTraceServerCall and pass in the list of ignored gRPC methods in as parameter:

/** * Returns true if the server call should not be traced. */ function shouldNotTraceServerCall( metadata: grpcJs.Metadata, methodName: string, ignoreGrpcMethods?: IgnoreMatcher[] ): boolean { const parsedName = methodName.split('/'); return ( containsOtelMetadata(metadata) || methodIsIgnored( parsedName[parsedName.length - 1] || methodName, ignoreGrpcMethods ) ); }

That being said, the original implementation is consistent with the rest of the package

When the first parameter is named this in typescript it carries special meaning. It only carries typing info and is compiled away. Example Playground Link to illustrate.

Please make this function a private member of the class.

I think maybe I still don't fully understand how to make this work. By moving the function over to class GrpcJsPlugin I get a compile error:

src/grpcJs.ts:107:11 - error TS6133: 'shouldNotTraceServerCall' is declared but its value is never read. 107 private shouldNotTraceServerCall( ~~~~~~~~~~~~~~~~~~~~~~~~ src/server/patchServer.ts:75:24 - error TS2341: Property 'shouldNotTraceServerCall' is private and only accessible within class 'GrpcJsPlugin'. 75 if (plugin.shouldNotTraceServerCall(call.metadata, name)) { ~~~~~~~~~~~~~~~~~~~~~~~~ Found 2 errors.

Do I need to change anything else in patchServer to make this work?

Just checking in to see if there's any advice on this issue?

Ah I see it is called from patchserver not from within the same class. You can make it a util function, but please do not use call to call it. It would be much better if you could pass any required configs or properties as arguments to the function.

obecny · 2020-08-06T15:07:12Z

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

+  originalFunc: HandleCall<RequestType, ResponseType>,
+  call: ServerCallWithMeta<RequestType, ResponseType>,
+  callback: SendUnaryDataCallback<unknown>
+): void {


Are you sure the return type of originalFunc will always be void ?

According to the grpc-js types, HandleCall is always going to be a void function

obecny · 2020-08-06T15:07:35Z

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

+export type IgnoreMatcher = string | RegExp | ((str: string) => boolean);
+
+export interface GrpcPluginOptions extends PluginConfig {
+  /* Omits tracing on any RPC methods that match any of


Suggested change

/* Omits tracing on any RPC methods that match any of

/* Omits tracing on any GRPC methods that match any of

obecny · 2020-08-06T15:10:53Z

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

+  /* Omits tracing on any RPC methods that match any of
+   * the IgnoreMatchers in the ignoreRpcMethods list
+   */
+  ignoreRpcMethods?: IgnoreMatcher[];


should this be called ignoreGrpcMethods ?

obecny · 2020-08-06T15:12:40Z

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

+    return false;
+  }
+
+  try {


please remove try catch, then function satisfiesPattern should simply return false instead of throwing an error

obecny · 2020-08-06T15:14:08Z

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

+ * @param ignoredMethods a list of matching patterns
+ * @param onException an error handler for matching exceptions
+ */
+export const _methodIsIgnored = (


this method should not throw anything, you can simply return boolean and make the same logic

dyladan · 2020-08-06T18:37:56Z

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

@@ -72,6 +73,15 @@ export function patchServer(
          ) {
            const self = this;

+            if (shouldNotTraceServerCall.call(plugin, call.metadata, name)) {


Use of call here is unnecessary and confusing. Making the function a private method of the class would be much better.

Suggested change

if (shouldNotTraceServerCall.call(plugin, call.metadata, name)) {

if (plugin.shouldNotTraceServerCall(call.metadata, name)) {

dyladan · 2020-08-06T18:38:38Z

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

+ * Returns true if the server call should not be traced.
+ */
+function shouldNotTraceServerCall(
+  this: GrpcJsPlugin,


Please make this function a private member of the class.

Signed-off-by: reggiemcdonald <[email protected]>

reggiemcdonald · 2020-08-17T19:22:41Z

Are there any more changes needed for this PR?

dyladan

Sorry this fell too far down my backlog and got a little lost. Looks good to me after the changes thanks

dyladan · 2020-08-19T15:25:25Z

@obecny and @vmarchaud have your concerns been addressed?

reggiemcdonald requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners July 27, 2020 22:34

markwolff reviewed Jul 29, 2020

View reviewed changes

reggiemcdonald mentioned this pull request Jul 31, 2020

feat: use gRPC to write spans to cloud trace GoogleCloudPlatform/opentelemetry-operations-js#97

Merged

vmarchaud approved these changes Aug 1, 2020

View reviewed changes

dyladan added the Internship Ending Soon label Aug 5, 2020

obecny reviewed Aug 6, 2020

View reviewed changes

dyladan reviewed Aug 6, 2020

View reviewed changes

reggiemcdonald added 10 commits August 12, 2020 15:38

feat: omit tracing for requests with otel outgoing metadata

51b8b32

Signed-off-by: reggiemcdonald <[email protected]>

feat: added ability to filter methods using plugin config

4b542a1

Signed-off-by: reggiemcdonald <[email protected]>

fix: add case insensitivity to method name matching

3c2601d

Signed-off-by: reggiemcdonald <[email protected]>

chore: update readme to include new config option

b263dff

Signed-off-by: reggiemcdonald <[email protected]>

chore(opentelemetry-plugin-grpc): add comment to constant

1030b1e

Signed-off-by: reggiemcdonald <[email protected]>

fix: addresss pr feedback by adding matchers to grpc plugin config

430696b

Signed-off-by: reggiemcdonald <[email protected]>

chore: update readme to reflect new config options

a3e38d1

Signed-off-by: reggiemcdonald <[email protected]>

fix: address pr feedback to rename config option

c36d08c

Signed-off-by: reggiemcdonald <[email protected]>

fix: address pr feedback to remove exception

65b1015

Signed-off-by: reggiemcdonald <[email protected]>

fix: name in comments

be3536f

Signed-off-by: reggiemcdonald <[email protected]>

reggiemcdonald added 2 commits August 12, 2020 15:38

fix: remove call

10b0078

Signed-off-by: reggiemcdonald <[email protected]>

chore: update readme

d2c67ec

Signed-off-by: reggiemcdonald <[email protected]>

reggiemcdonald force-pushed the 1327-filter-exporter-requests branch from 91d23df to d2c67ec Compare August 12, 2020 17:00

dyladan approved these changes Aug 17, 2020

View reviewed changes

markwolff approved these changes Aug 19, 2020

View reviewed changes

obecny approved these changes Aug 19, 2020

View reviewed changes

obecny and others added 2 commits August 19, 2020 19:04

Merge branch 'master' into 1327-filter-exporter-requests

068a6b7

Merge branch 'master' into 1327-filter-exporter-requests

a6f30e5

dyladan merged commit b2eedfb into open-telemetry:master Aug 20, 2020

dyladan mentioned this pull request Sep 2, 2020

plugin-grpc does not filter outgoing requests from exporter #1327

Closed

2 tasks

marcoreni mentioned this pull request Sep 7, 2020

Massive amount of spans ingested GoogleCloudPlatform/opentelemetry-operations-js#117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to filter grpc methods #1353

feat: add ability to filter grpc methods #1353

reggiemcdonald commented Jul 27, 2020

codecov bot commented Jul 27, 2020 •

edited

Loading

reggiemcdonald commented Jul 27, 2020

markwolff left a comment

markwolff Jul 29, 2020

reggiemcdonald Jul 29, 2020

markwolff Jul 29, 2020

reggiemcdonald Jul 31, 2020

dyladan Jul 31, 2020

markwolff Jul 29, 2020

reggiemcdonald Jul 29, 2020

reggiemcdonald commented Jul 29, 2020 •

edited

Loading

dyladan commented Jul 29, 2020

reggiemcdonald commented Jul 29, 2020

obecny Aug 6, 2020

reggiemcdonald Aug 6, 2020 •

edited

Loading

obecny Aug 6, 2020

reggiemcdonald Aug 6, 2020 •

edited

Loading

dyladan Aug 6, 2020

dyladan Aug 6, 2020

reggiemcdonald Aug 6, 2020 •

edited

Loading

reggiemcdonald Aug 12, 2020

dyladan Aug 12, 2020

obecny Aug 6, 2020

reggiemcdonald Aug 6, 2020 •

edited

Loading

obecny Aug 6, 2020

obecny Aug 6, 2020

obecny Aug 6, 2020

obecny Aug 6, 2020

dyladan Aug 6, 2020

dyladan Aug 6, 2020

reggiemcdonald commented Aug 17, 2020

dyladan left a comment

dyladan commented Aug 19, 2020

	export const satisfiesPattern = <T>(
	constant: string,
	pattern: IgnoreMatcher
	): boolean => {
	if (typeof pattern === 'string') {
	return pattern === constant;
	} else if (pattern instanceof RegExp) {
	return pattern.test(constant);
	} else if (typeof pattern === 'function') {
	return pattern(constant);
	} else {
	throw new TypeError('Pattern is in unsupported datatype');
	}
	};

	/* Omits tracing on any RPC methods that match any of
	/* Omits tracing on any GRPC methods that match any of

	if (shouldNotTraceServerCall.call(plugin, call.metadata, name)) {
	if (plugin.shouldNotTraceServerCall(call.metadata, name)) {

feat: add ability to filter grpc methods #1353

feat: add ability to filter grpc methods #1353

Conversation

reggiemcdonald commented Jul 27, 2020

Which problem is this PR solving?

Short description of the changes

Testing

codecov bot commented Jul 27, 2020 • edited Loading

Codecov Report

reggiemcdonald commented Jul 27, 2020

markwolff 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

reggiemcdonald commented Jul 29, 2020 • edited Loading

dyladan commented Jul 29, 2020

reggiemcdonald commented Jul 29, 2020

Choose a reason for hiding this comment

reggiemcdonald Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reggiemcdonald Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reggiemcdonald Aug 6, 2020 • edited Loading

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

reggiemcdonald Aug 6, 2020 • edited Loading

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

reggiemcdonald commented Aug 17, 2020

dyladan left a comment

Choose a reason for hiding this comment

dyladan commented Aug 19, 2020

codecov bot commented Jul 27, 2020 •

edited

Loading

reggiemcdonald commented Jul 29, 2020 •

edited

Loading

reggiemcdonald Aug 6, 2020 •

edited

Loading

reggiemcdonald Aug 6, 2020 •

edited

Loading

reggiemcdonald Aug 6, 2020 •

edited

Loading

reggiemcdonald Aug 6, 2020 •

edited

Loading