fix(amqplib): stop importing from amqplib directly in compiled types

[dotboris]

Which problem is this PR solving?

I started getting type checking errors when using node auto instrumentation:

$ tsc --skipLibCheck false --incremental false
node_modules/@opentelemetry/instrumentation-amqplib/build/src/utils.d.ts:3:28 - error TS2307: Cannot find module 'amqplib' or its corresponding type declarations.

3 import type * as amqp from 'amqplib';
                             ~~~~~~~~~


Found 1 error in node_modules/@opentelemetry/instrumentation-amqplib/build/src/utils.d.ts:3

Turns out that the compiled utils.d.ts imports amqplib directly which breaks. This worked fine before #1320 which moved @types/amqplib as a dev dependency. The PR adjusted most places where amqplib was imported but it missed one.

Short description of the changes

• Make getConsumePatch, getConfirmedPublishPatch and getPublishPatch in AmqplibInstrumentation private. They used to be protected.
• Reduce the number of vendor'd types in types.ts

The generate amqplib.d.ts now looks like this: (no longer imports ./utils which imports amqplib)

import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
import { AmqplibInstrumentationConfig } from './types';
export declare class AmqplibInstrumentation extends InstrumentationBase {
    protected _config: AmqplibInstrumentationConfig;
    constructor(config?: AmqplibInstrumentationConfig);
    setConfig(config?: AmqplibInstrumentationConfig): void;
    protected init(): InstrumentationNodeModuleDefinition<unknown>;
    private patchConnect;
    private unpatchConnect;
    private patchChannelModel;
    private unpatchChannelModel;
    private getConnectPatch;
    private getChannelEmitPatch;
    private getAckAllPatch;
    private getAckPatch;
    private getConsumePatch;
    private getConfirmedPublishPatch;
    private getPublishPatch;
    private createPublishSpan;
    private endConsumerSpan;
    private endAllSpansOnChannel;
    private callConsumeEndHook;
    private checkConsumeTimeoutOnChannel;
}
//# sourceMappingURL=amqplib.d.ts.map

[codecov]

Codecov Report

Merging #1394 (7682ad0) into main (00cc8d6) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1394   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files          14       14           
  Lines         898      898           
  Branches      192      192           
=======================================
  Hits          863      863           
  Misses         35       35           

[Flarna]

Failed test is unrelated => lerna/lerna#3536

[blumamir]

Thanks for reporting this.
The fix LGTM, but I think the real issue is that utils.d.ts is exported in the public interface where it shouldn't

amqplib.d.ts:

import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
import { AmqplibInstrumentationConfig, ConsumeMessage, Options, Replies } from './types';
import { InstrumentationConsumeChannel, InstrumentationPublishChannel } from './utils';
export declare class AmqplibInstrumentation extends InstrumentationBase {
    protected _config: AmqplibInstrumentationConfig;
    constructor(config?: AmqplibInstrumentationConfig);
    setConfig(config?: AmqplibInstrumentationConfig): void;
    protected init(): InstrumentationNodeModuleDefinition<unknown>;
    private patchConnect;
    private unpatchConnect;
    private patchChannelModel;
    private unpatchChannelModel;
    private getConnectPatch;
    private getChannelEmitPatch;
    private getAckAllPatch;
    private getAckPatch;
    protected getConsumePatch(moduleVersion: string | undefined, original: Function): (this: InstrumentationConsumeChannel, queue: string, onMessage: (msg: ConsumeMessage | null) => void, options?: Options.Consume | undefined) => Promise<Replies.Consume>;
    protected getConfirmedPublishPatch(moduleVersion: string | undefined, original: Function): (this: InstrumentationConsumeChannel, exchange: string, routingKey: string, content: Buffer, options?: Options.Publish | undefined, callback?: ((err: any, ok: Replies.Empty) => void) | undefined) => boolean;
    protected getPublishPatch(moduleVersion: string | undefined, original: Function): (this: InstrumentationPublishChannel, exchange: string, routingKey: string, content: Buffer, options?: Options.Publish | undefined) => boolean;
    private createPublishSpan;
    private endConsumerSpan;
    private endAllSpansOnChannel;
    private callConsumeEndHook;
    private checkConsumeTimeoutOnChannel;
}
//# sourceMappingURL=amqplib.d.ts.map

I think a possible alternative fix will be to make the getConsumePatch, getConfirmedPublishPatch and getPublishPatch functions private (I don't see a good reason for them to be protected anyway). That would remove them from the public API which will also remove the import { InstrumentationConsumeChannel, InstrumentationPublishChannel } from './utils'; part

This will reduce the number of types we need to copy into the instrumentation code, which hopefully can make it a bit simpler to maintain. WDYT? @dotboris @Flarna

[Flarna]

moving more APIs to private sounds good. can be also done as followup depending on @dotboris motivation to rework this PR.

[blumamir]

can be also done as followup depending on @dotboris motivation to rework this PR.

If I got it right, that would mean we don't need to vendor any new amqplib type in types.ts in this PR (export interface Channel extends EventEmitter { etc).

[dotboris]

I'll give making the methods private a shot to see how it goes.

[blumamir]

I'll give making the methods private a shot to see how it goes.

Thanks for updating the PR. Do we still need the changes in types.ts? (all the new vendored types)

[dotboris]

Thanks for updating the PR. Do we still need the changes in types.ts? (all the new vendored types)

I have reverted the additional types that I previously vendored. I've also removed a bunch of types that did not need to be there anymore. The rest of the vendored types are required because they're used as part of the instrumentation config.

To answer your question more directly: yes we still need them.

[blumamir]

LGTM, thanks for the fix 🙏