InstrumentationBase calls init on partly initialized Instrumentations

[Flarna]

What version of OpenTelemetry are you using?

0.17.0

What version of Node are you using?

14.16.0

What did you do?

see open-telemetry/opentelemetry-js-contrib#354 (comment)

Implementing an Instrumentation which uses a private property in init().

What did you expect to see?

Instrumentation should work

What did you see instead?

init() was called before the private property was initialized.

Additional context

• InstrumentationAbstract declares abstract init()
• init() most be implemented in a concrete instrumentation class extending InstrumentationBase
• constructor of InstrumentationBase calls this.init()

==> This result in calling init() of the concrete Instrumentation instance before the constructor has finished. The functions of this instrumentation are in place but the properties are not yet initialized resulting in undefined behavior.

Refs.: microsoft/TypeScript#9209

[dyladan]

@Flarna I believe this has been fixed. Can you confirm if this is still a bug?

[Flarna]

I don't think it's fixed as abstract init is still called from base class constructor here

[david-luna]

Hi,

As @Flarna mentioned this is not fixed yet and I think we would might want to challenge the design on how the Instrumentations are enabled/started to avoid more issues in the future. Here are my questions and thoughts about it. Bear in mind I wasn't there when this was designed and probably the context was different so the following observations are based in my knowledge of the project.

Auto registration of Hooks upon constructor
The logic inside the abstract constructor is for adding the RITM/IITM hooks for patching the libraries. That's convenient because we want to have this setup before any require/import statement from the application. We can also argue this "autowiring" is good for development experience.

I've checked all documentation sources I know (website and *.md files from repos) and there is no example of using any instrumentation in isolation. The documented way of instrument an application is through the SDK (or auto instrumentations which uses the SDK under the hood) which actually does the wiring of many things: processors, exporters, readers, etc. The SDK also should be started before any require/import from the application.

Why not then let the SDK signal the instrumentations when they should add their hooks? If we do so the logic could be placed in a different function rather than the constructor, breaking this parent-child constructor cycle (child calling super and parent calling child methods)

Patch/unpatch logic exposed to overrides
Maybe there's is not much we can do here but there is a chance that a developer might want to have some logic upon enabling/disabling the instrumentation. So the 1st thing she might try is just to override the enable method like I did on the undici instrumentation. In my case there was no patching needed so I works anyway but instrumentations that rely on patching will stop working if the method is overwritten without calling super.enable().

I guess a note in the dev guidelines compelling to call the super class function would suffice but I wonder if we could remove any chance of error. One possibility I see is to consider that an instrumentation have a lifecycle with at least 2 states: enabled, disabled. And provide onEnabled and onDisabled hooks for devs to add the logic they need.

Going back to this concrete issue @trentm and I though about using process.nextTick to ensure the child class did initialize all properties so the init/enable logic within the super class constructor runs after all properties are initialized.

Here is an example of what we think it could fix the issue. IMO is a workaround and we should consider to solve the design issue for 2.0

https://www.typescriptlang.org/play/?#code/CYUwxgNghgTiAEA3W8AOMD2YQGccC54oA7ATwG4AoSqAIxwBcYowH5Io94BBeplhgGFoXAN6V47DMUYwArqwwwAFAEp44yZPRZcOAHTEQADwYAVAJZgA1srXwAvAD4NErZIYALCwYvELDGpU7gC+qm4hbnSyAvB+AWqEJBSUkZQcXILSYHAMIMKcOPAmecTARbwxrAVibjqoIDAMpITEcgC2tI1UbmDSsgoMSvaaWjhyDSqqwVpePvr1jc2O8AAsM2mS8YGqSWSu7n0yGBAg+hAYAObKcwaLTaTTEanUR4zsK0YA7vBZxDkgPI1HBBShAA

[david-luna]

@Flarna @dyladan

This is not the only issue with the current design. I've just created open-telemetry/opentelemetry-js-contrib#2320 and I think we should discuss this in the SIG

[david-luna]

@Flarna @dyladan @pichlermarc @trentm

This issue has been stalled for a while and I think it would be good to give it another try before the next major release 2.0.

I would propose something that has been done lately to avoid coupling and other issues for consumers. I think we should export an interface instead of an abstract class for instrumentations like we did in

• feat(sdk-metrics)!: extract IMetricReader interface and use it over abstract class #5311
• refactor(sdk-trace-base)!: remove Span class from exports #5048

I've checked most of the instrumentations and they mainly use the following from the super class:

• wrapper functions (_wrap, _unwrap, _massWrap, _massUnwrap)
• config
• tracer, meter & diag logger

The instrumentation provided by the dev is not participating in the registration of RITM/IITM hooks rather than provide the definitions with the patch & unpatch methods. I think is possible to decouple both parts resulting on an internal class or method managing the hook/unhook process and injecting there dependencies into the instrumentation object that implements the interface.

I see 2 possible options

• use the delegate pattern. SDK builds creates instrumentations which have a delegate object provided by the dev which implements a given interface. The instrumentation provides the current API (setTracerProvider, enable, disable) and calls the delegate methods in some situations
• use a factory function which receives objects with the given interface and decorates them to have the missing parts.

Both options could be part of the registerInstrumentations process making it easy to migrate for devs. If the instrumentation is an instance of the InstrumentationBase class we do nothing, otherwise we apply the wrapper class or function to the instrumentation.

registerInstrumentations({ instrumentations: [new HttpInstrumentation()] })

One side effect would be that for new instrumentations the hooking logic would be delayed until registerInstrumentations is called. This is another topic we discussed som times but without reaching any conclusion.

I'm not sure I have done a good explanation so I'm going to prepare a POC with option 1 (delegate) and share it here so you can review and provide feedback

[pichlermarc]

@david-luna given that @opentelemetry/instrumentation is experimental (0.x), which means we can make that change after we've done the 2.x release, what do you think about postponing the work on this to after 2.x.?

I have already proposed this as a future focus topic. This way we can ensure these changes get the attention they deserve (refs #5149, #4586, edit: and #4839).

That being said, I think your proposal makes sense - I feel like our ideas on where this align quite well :)

[david-luna]

@david-luna given that @opentelemetry/instrumentation is experimental (0.x), which means we can make that change after we've done the 2.x release, what do you think about postponing the work on this to after 2.x.?

I have already proposed this as a future focus topic. This way we can ensure these changes get the attention they deserve (refs #5149, #4586, edit: and #4839).

That being said, I think your proposal makes sense - I feel like our ideas on where this align quite well :)

@pichlermarc I think is a good idea. Sorry for not checking all the issues

I'll do a draft PR and link it to the issues for further analysis/discussion. Thanks :)

[pichlermarc]

@david-luna given that @opentelemetry/instrumentation is experimental (0.x), which means we can make that change after we've done the 2.x release, what do you think about postponing the work on this to after 2.x.?
I have already proposed this as a future focus topic. This way we can ensure these changes get the attention they deserve (refs #5149, #4586, edit: and #4839).
That being said, I think your proposal makes sense - I feel like our ideas on where this align quite well :)

@pichlermarc I think is a good idea. Sorry for not checking all the issues

No worries, they're kind of all over the place and rough at the moment.

Once we're done with 2.0, let's sync and come up with a more detailed plan based on your prototype - I think improving the instrumentation package and eventually marking it as stable is a key piece that holds the ecosystem back, especially around the maintainability of instrumentations that are maintained outside of the contrib repo, but also for ourselves here in core and contrib. We should tackle this soon-ish. 👍