[instrumentation] hide `shimmer` types from the public API

[pichlermarc]

Description:

@types/shimmer is currently part of the @opentelemetry/instrumentation package's public API. We should avoid having a third-party dependency be part of the public API of this package as breaking changes will affect the public interface of @opentelemetry/instrumentation

A solution to this can be changing InstrumentationAbstract#_wrap, InstrumentationAbstract#_unwrap, InstrumentationAbstract#massUnwrap, InstrumentationAbstract#massWrap to be custom functions that wrap shimmer functions and for which we fully control the types.

This issue is considered done when:

• shimmer, @types/shimmer exports are hidden in a way so that they are not part of the public API anymore
• @types/shimmer is moved to devDependenices or removed completely

Additional Details

Code sections of interest:

• opentelemetry-js/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts

  Lines 73 to 80 in 01a2c35

  	/* Api to wrap instrumented method */
  	protected _wrap = shimmer.wrap;
  	/* Api to unwrap instrumented methods */
  	protected _unwrap = shimmer.unwrap;
  	/* Api to mass wrap instrumented method */
  	protected _massWrap = shimmer.massWrap;
  	/* Api to mass unwrap instrumented methods */
  	protected _massUnwrap = shimmer.massUnwrap;

• opentelemetry-js/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

  Lines 82 to 152 in 01a2c35

  	protected override _wrap: typeof wrap = (moduleExports, name, wrapper) => {
  	if (isWrapped(moduleExports[name])) {
  	this._unwrap(moduleExports, name);
  	}
  	if (!utilTypes.isProxy(moduleExports)) {
  	return wrap(moduleExports, name, wrapper);
  	} else {
  	const wrapped = wrap(Object.assign({}, moduleExports), name, wrapper);
  	return Object.defineProperty(moduleExports, name, {
  	value: wrapped,
  	});
  	}
  	};
  	protected override _unwrap: typeof unwrap = (moduleExports, name) => {
  	if (!utilTypes.isProxy(moduleExports)) {
  	return unwrap(moduleExports, name);
  	} else {
  	return Object.defineProperty(moduleExports, name, {
  	value: moduleExports[name],
  	});
  	}
  	};
  	protected override _massWrap: typeof massWrap = (
  	moduleExportsArray,
  	names,
  	wrapper
  	) => {
  	if (!moduleExportsArray) {
  	diag.error('must provide one or more modules to patch');
  	return;
  	} else if (!Array.isArray(moduleExportsArray)) {
  	moduleExportsArray = [moduleExportsArray];
  	}
  	if (!(names && Array.isArray(names))) {
  	diag.error('must provide one or more functions to wrap on modules');
  	return;
  	}
  	moduleExportsArray.forEach(moduleExports => {
  	names.forEach(name => {
  	this._wrap(moduleExports, name, wrapper);
  	});
  	});
  	};
  	protected override _massUnwrap: typeof massUnwrap = (
  	moduleExportsArray,
  	names
  	) => {
  	if (!moduleExportsArray) {
  	diag.error('must provide one or more modules to patch');
  	return;
  	} else if (!Array.isArray(moduleExportsArray)) {
  	moduleExportsArray = [moduleExportsArray];
  	}
  	if (!(names && Array.isArray(names))) {
  	diag.error('must provide one or more functions to wrap on modules');
  	return;
  	}
  	moduleExportsArray.forEach(moduleExports => {
  	names.forEach(name => {
  	this._unwrap(moduleExports, name);
  	});
  	});
  	};

This issue is part of #4586

[cjihrig]

@pichlermarc instead of wrapping shimmer, would you be open to porting it directly into the codebase? The entire implementation is just a bit over 100 lines, and a new version hasn't been released in 6 years, so it can probably be considered stable. That would allow both the shimmer and @types/shimmer dependencies to be dropped.

[pichlermarc]

@pichlermarc instead of wrapping shimmer, would you be open to porting it directly into the codebase?

Absolutely.

I even had started some work on this where I had broken it out into a package provided by us - @opentelemetry/shimmer: that's the only approach I'd like to avoid. There's no need to have it be a separate package, but I did it under time-pressure as we had a security scare that fortunately turned out to be unfounded. You may be able to re-use some of that work when integrating it into @opentelemetry/instrumentation though, here's a link to the commit on my fork if you're interested - adapting the tests from tap to mocha took some time to do. 🙂

The entire implementation is just a bit over 100 lines, and a new version hasn't been released in 6 years, so it can probably be considered stable. That would allow both the shimmer and @types/shimmer dependencies to be dropped.

Yes, shimmer is probably as 'done' as any piece of software can be. Licensing concerns might cause some friction. shimmer is using BSD-2-clause, and @types/shimmer is using MIT. Both should be compatible with Apache-2 (which we use), so we should be fine, but a PR might need to jump through some more hoops than usual to get it approved.

Feel free to go ahead if you want to work on this 🙂

[cjihrig]

I started looking into this. It appears that there are a few discrepancies between the types and the implementation of wrap(). The signature in DefinitelyTyped is:

wrap<Nodule extends object, FieldName extends keyof Nodule>(
    nodule: Nodule,
    name: FieldName,
    wrapper: (original: Nodule[FieldName]) => Nodule[FieldName],
): void;

• The return type is documented as void, but there is a return value in the implementation.
• The wrapper argument is defined as a function taking a single argument. However, shimmer has a test validating that the function accepts two arguments. This test is not only inconsistent with the types, but also with the shimmer README, which states:

wrapper is a function that takes a single parameter, which is the original function to be monkeypatched.

What is the best way to proceed here? Should the types be changed to match the implementation as a bug fix, or would that be considered a breaking change?

[pichlermarc]

What is the best way to proceed here? Should the types be changed to match the implementation as a bug fix, or would that be considered a breaking change?

I think the route we should take is:

• if we don't need it, we align our shimmer implementation's feature set to the DefinitelyTyped signature, later on if we see that it's needed we can still introduce it as a new feature.
  • I think this is the most likely route, this keeps API surface small and leaves options for us in the future. Anyone that needs more can still take a direct dependency on shimmer in the meantime.
• if we need it, we adapt to use the type that matches what shimmer does - that's technically a breaking change, but I assume impact would be fairly minimal. The @opentelemetry/instrumentation package is also still 0.x so breaking changes are to be expected. The way most end-users consume instrumentation is via the Instrumentation interface, which would not change.