Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!(instrumentation): remove moduleExports generic type from instrumentation registration #4598

Merged
merged 18 commits into from
Apr 19, 2024

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Mar 31, 2024

This PR removes the generic type on InstrumentationBase which determines the moduleExports types for patching and unpatching.

Adding types for function parameters, variables and return values is considered idiomatic and good practice in typescript. Thus it was added to the OpenTelemetry InstrumentationBase class in the first place - to promote type safety. However, while it has benefits, there are also some downsides which we observed over the last years:

Public Package API

the generic parameter can leak private types to the public api.

A typical instrumentation that uses this generic looks like this:

import type { FooType } from 'foo';
export default class FooInstrumentation extends InstrumentationBase<FooType> {
...
    init(): InstrumentationNodeModuleDefinition<FooType> {
        return new InstrumentationNodeModuleDefinition<FooType>(
            ...,
            moduleExports => { // moduleExports is typed by the generic as FooType
                 ....
            }
        )
    }

}

After typescript transpilation, this public init function remains in the index.d.ts transitive import, which means that any typescript user of the instrumentation must satisfy import type { FooType } from 'foo'; and have the types installed as part of the dependencies.

This has proven to be problematic as:

  1. we cannot assume that the instrumented package itself (foo in the example) is found in the user application, as it is a valid use case to install instrumentations for packages which are not a dependency (as happens, for example, with the auto-instrumentation package).
  2. we don't want to install the package itself as a dependency of the instrumentation, as it might introduce an overhead on the node_modules which is unacceptable.
  3. even if we have a @types available (e.g. @types/foo in our example), the types package is not always well maintained, compatible, or stable.

To overcome the above issues, it is common for instrumentations to drop the type checking services for moduleExports and use any as the genric parameter.
Few statistics:

  • use any 22 instrumentations - (amqplib, cucumber, dataloader, lru-memoizer, mongoose, socket.io, aws-lambda, aws-sdk, cassandra, fastify, generic-pool, graphql, ioredis, knex, mongodb, mysql2, nestjs, pino, redis, restify, router, winston)
  • use the generic type with @types/ dependency 10 instrumentations - (fs, tedious, bunyan, connect, express, hapi, koa, memcached, mysql, pg)
  • vendor the types internally 2 instrumentations- (dns, net).

This change will affect 12 existing instrumentations which will need to be updated.

Type Safety

Instrumentation can still cast the parameters in the InstrumentationModuleDefinition patch function to promote type safety.

For example:

    return new InstrumentationNodeModuleDefinition(
      'https',
      ['*'],
      (moduleExports: Https): Https => {

The patch signature is now: public patch?: (exports: any, moduleVersion?: string) => any, and implementations can use the right types ((exports: Https, moduleVersion?: string) Https) instead of any to use the correct type. The downside is that it is not automatically enforced and is more "OPT-IN". an instrumentation implementer would need to explicitly state the types instead of using the generic type.

Breaking Change

This change would require a small change in each existing instrumentation - removing the generic parameter and optionally adding type to the patch and unpatch functions of InstrumentationModuleDefinition and InstrumentationModuleFile. Since the package is experimental, it's ok to make such changes and publish them as a minor version bump.

Misc

Pros:

  • less chance for an instrumentation implementer to accidentally include a package in the public API. we have no assertions or automatic verifications of this problem, and it is one that is very easy to make. This change will me the issue less likely for future instrumentation authors and maintainers.
  • simpler API - the instrumentation API will become lighter, requiring fewer types and a mental understanding of nuances.

Cons:

  • to benefit from type safety in the instrumentation patch and unpatch functions, the user would need to specifically add the type to the function signature.

@blumamir blumamir requested a review from a team March 31, 2024 08:25
@blumamir blumamir marked this pull request as draft March 31, 2024 08:25
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

Merging #4598 (6745618) into main (2610122) will increase coverage by 2.07%.
Report is 2 commits behind head on main.
The diff coverage is 72.22%.

❗ Current head 6745618 differs from pull request most recent head 872853d. Consider uploading reports for the commit 872853d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4598      +/-   ##
==========================================
+ Coverage   90.77%   92.85%   +2.07%     
==========================================
  Files          90      328     +238     
  Lines        1930     9499    +7569     
  Branches      417     2042    +1625     
==========================================
+ Hits         1752     8820    +7068     
- Misses        178      679     +501     
Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 95.05% <100.00%> (ø)
...emetry-instrumentation-grpc/src/instrumentation.ts 92.09% <ø> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 93.37% <100.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 97.63% <100.00%> (ø)
...entelemetry-instrumentation/src/instrumentation.ts 80.48% <100.00%> (ø)
...strumentation/src/instrumentationNodeModuleFile.ts 33.33% <33.33%> (ø)
...ntation/src/instrumentationNodeModuleDefinition.ts 14.28% <25.00%> (ø)

... and 237 files with indirect coverage changes

@blumamir blumamir marked this pull request as ready for review March 31, 2024 11:45
@blumamir blumamir requested a review from Flarna March 31, 2024 11:45
@pichlermarc pichlermarc changed the title feat!(instrumentation): remove moudleExports generic type from instrumentation registration feat!(instrumentation): remove moduleExports generic type from instrumentation registration Apr 4, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, this generally this looks good. 👍
I think this plays well into #4586.

FYI @open-telemetry/javascript-approvers, this is a quite substantial change with a large impact on instrumentation authors. Please put your objections on the PR if you have any so that we can discuss further

blumamir and others added 7 commits April 14, 2024 10:26
…rm/node/instrumentation.ts

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
…rm/node/instrumentation.ts

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
@blumamir
Copy link
Member Author

Thank you @pichlermarc for the review.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I left a few suggestions around eslint warnings to add and remove, but those are nits and the general changes here look good.

name,
baseDir
);
return this._onRequire<typeof exports>(module, exports, name, baseDir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now looks so much cleaner and easier to reason about!

blumamir and others added 4 commits April 17, 2024 19:45
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
…mentationNodeModuleFile.ts

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
…mentationNodeModuleDefinition.ts

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
@blumamir
Copy link
Member Author

Thanks for working on this! I left a few suggestions around eslint warnings to add and remove, but those are nits and the general changes here look good.

Great suggestions, I need to be more aware of the eslint directives. Thank you for adding them

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…mentation registration (open-telemetry#4598)

* feat!(instrumentation): remove moudleExports generic type from instrumentation registration

* fix: lint

* chore: add changelog

* fix: core instrumentations

* docs: update README with the change

* Update experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* Update experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* Update CHANGELOG.md

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

* chore: lint

* revert: sdk-logs in tsconfig

* chore: lint markdown

* Apply suggestions from code review

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* Update experimental/packages/opentelemetry-instrumentation/src/instrumentationNodeModuleFile.ts

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* Update experimental/packages/opentelemetry-instrumentation/src/instrumentationNodeModuleDefinition.ts

Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>

* fix: remove unrelevant eslint ignore

---------

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants