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

Remove the mysql2 package peer dependency #695

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

tombruijn
Copy link
Member

@tombruijn tombruijn commented Jun 21, 2022

This peer dependency was added to fix the build for the OpenTelemetry
mysql2 package instrumentation. In the TypeScript build it would import
the mysql2 types in the @opentelemetry/instrumentation-mysql2 package.

The @opentelemetry/instrumentation-mysql2 package itself has no
dependency on the mysql2 package, and neither does
@opentelemetry/auto-instrumentations-node. Which got me interested to
find out how that package works if we're experiencing the problem
described in PR #688

Control packages peer dependencies
We need mysql2 as a peer dependency of the package because the
OpenTelemetry instrumentation library requires it. This could be fixed
with introspection using require-in-the-middle.

Instead I've moved it to the devDependencies list to fix the build and
remove the dependency for users that do not use the mysql2 package in
their app.

The AppSignal package works without it as a peer dependency, even if the
mysql2 package is not a dependency of the user's app. I've also tested
this in a TypeScript app without any conditional loading for the
@opentelemetry/instrumentation-mysql2 package.

@tombruijn tombruijn self-assigned this Jun 21, 2022
@tombruijn tombruijn force-pushed the mysql2-dependency branch from dcc8be8 to 378b3d6 Compare June 21, 2022 11:14
@tombruijn tombruijn marked this pull request as ready for review June 21, 2022 11:21
@tombruijn tombruijn force-pushed the mysql2-dependency branch from 378b3d6 to d9fde59 Compare June 21, 2022 11:24
Copy link
Member

@luismiramirez luismiramirez left a comment

Choose a reason for hiding this comment

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

👍🏼

Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

Looks good to me! Koa stuff has already been merged, so we can merge this to the opentelemetry branch instead.

This peer dependency was previously added to fix the build for the
OpenTelemetry mysql2 package instrumentation. In the TypeScript build of
this package it would import the mysql2 types in the
`@opentelemetry/instrumentation-mysql2` package, which we import.

The `@opentelemetry/instrumentation-mysql2` package itself has no
runtime dependency on the mysql2 package, and neither does
`@opentelemetry/auto-instrumentations-node`. Which got me interested to
find out how that package works if we're experiencing the problem
described in PR #688

> Control packages peer dependencies
> We need mysql2 as a peer dependency of the package because the
> OpenTelemetry instrumentation library requires it. This could be fixed
> with introspection using require-in-the-middle.

Instead I've moved it to the devDependencies list to fix the build and
remove the dependency for users that do not use the mysql2 package in
their app.

I've then tested it in a TypeScript example app
appsignal/opentelemetry#29 that doesn't use
mysql2. The AppSignal package works without mysql2 as a peer dependency
in this scenario.
@tombruijn tombruijn force-pushed the mysql2-dependency branch from d9fde59 to 68ab71b Compare June 22, 2022 06:37
@tombruijn tombruijn changed the base branch from opentelemetry-koa-instrumentation to opentelemetry June 22, 2022 06:37
@tombruijn tombruijn merged commit a1d947c into opentelemetry Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants