GrpcInstrumentation does not implement @opentelemetry/instrumentation InstrumentationBase

[KaydenMiller]

Context

As far as I can tell the other @opentelemetry/<instrumentation-name-here> libraries all implement the InstrumentationBase<any> interface from the @opentelemetry/instrumentation library. Including many of the 3rd party libraries, such as for koa, MongoDb, and others.

Ran into this particular problem when trying to work with the types for multiple instrumentation tools for my work project where we needed to provide overrides or create new instances when no override was available. Given the GRPC Instrumentation didn't use the interface of the others I wanted to see if this was intentional or not.

Question

Should the GrpcInstrumentation library also follow this convention or is there a reason why it does not use the InsturmentationBase interface like many of the others?

Example

You can see an example of this in the following files

• https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
• https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
• https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts

where it is different in this one:

• https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts

• This only affects the JavaScript OpenTelemetry library

• This may affect other libraries, but I would like to get opinions here first

[Flarna]

grpc instrumenation has some history. Initially grpc and grpcjs had separate instrumentation libraries. Not 100% sure but I guess that time both of them used InstrumentationBase (if it was a thing already that time).

• Later they were combined into one instrumenation for convenience reasons, see feat: merge grpc-js into grpc instrumentation #1657 #1806 InstrumentationBase was used of both internally.
• in chore(instrumentation-grpc): drop support for grpc #3807 grpc support was removed as grpc went out of support. Code was removed but cleanup didn't go all the way down to remove the wrapper class GrpcInstrumentation

As far as I can see the GrpcInstrumentation offers everything InstrumentationBase does. Actually all calls each call is forwarded to the internal GrpcJsInstrumentation.

Do you miss anything specific or does the GrpcInstrumentation behave in a way which is incorrect?

FWIW I think a cleanup step to remove the GrpcInstrumentation wrapper class would be a nice to have.

[KaydenMiller]

No as far as I can tell the functionality is there and works just fine. This particular issue is when you tell Typescript that you have for example a list of InstrumentationBase classes it will error out a the type checking level due to the GRPC instrumentation not having implemented that.

Here is a screenshot of the particular error:

The properties that the interface states are required are not present on the GRPC Instrumentation and so Typescript will not let you use it as an InstrumentationBase with all of the other Instrumentation classes. I should be able to just ignore it using @ts-ignore and it should still work (though I haven't tested that in this particular case yet). Though I would rather avoid using a ts-ignore where possible so that I continue to get the type checking that I'm expecting.

Does that make sense?

[Flarna]

Ah yes, makes sense. So actually more a typescript caused issue then a functional one.

I think typescript leaks to much of the internals here. A more compatible solution would be to export an interface InstrumentationApi (or whatever name we user here). InstrumentationBase implements this API and therefore all derived classes get it.
For special cases like the GrpcInstrumentation one should directly implement the API instead of using InstrumentationBase base class.
So in your case you could use this interface type instead of the base class.

We had similar issues caused by typescript in the past. @dyladan do we have the refactoring to move towards interfaces on the SDK 2.0 roadmap?

But as said above for the GrpcInstrumentation this bigger change is actually not needed. The current situation is more a leftover and it's just waiting on someone to cleanup and use InstrumentationBase as base class.

[KaydenMiller]

Ah yeah, also I just realized that ImplementationBase was an abstract class not an interface itself, thats my bad I should have double checked that rather than making an assumption there. But yeah some sort of InstrumentationApi interface or shared abstract class would work just fine for my needs.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue was closed because it has been stale for 14 days with no activity.

[pichlermarc]

FWIW I think a cleanup step to remove the GrpcInstrumentation wrapper class would be a nice to have.

I'll open a PR to do that, it's dead unnecessary code and I think it's time we remove it.