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 nodejs autoinstrumentation dev dependencies from image #2803

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

Zirak
Copy link
Contributor

@Zirak Zirak commented Mar 31, 2024

Description:

The autoinstrumentation has several build-time dependencies which aren't necessary for the final image, like typescript. Comparison of builds without typescript, typescript + rimraf, and finally all dev dependencies:

$ docker image ls --format 'table {{.Repository}} {{.Tag}} {{.Size}}'
autoinstrumentation-nodejs 60d37e18ce 272MB
autoinstrumentation-nodejs no-ts 208MB
autoinstrumentation-nodejs no-ts-rmrf 201MB
autoinstrumentation-nodejs no-dev 200MB

We extend the existing postcompile script. After building into build/workspace, we prune to remove extraneous packages. Doing that requires the package.json in the build directory.

Link to tracking Issue(s):

Testing: Locally instrumenting a nodejs application.

The autoinstrumentation has several build-time dependencies which aren't
necessary for the final image, like typescript. Comparison of builds without
typescript, typescript + rimraf, and finally all dev dependencies:

```
$ docker image ls --format 'table {{.Repository}} {{.Tag}} {{.Size}}'
autoinstrumentation-nodejs 60d37e1 272MB
autoinstrumentation-nodejs no-ts 208MB
autoinstrumentation-nodejs no-ts-rmrf 201MB
autoinstrumentation-nodejs no-dev 200MB
```

We extend the existing `postcompile` script. After building into
`build/workspace`, we `prune` to remove extraneous packages. Doing that requires
the `package.json` in the build directory.
@Zirak Zirak requested a review from a team March 31, 2024 06:41
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM, but someone with a better understanding of the node instrumentation should double-check before we merge.

I do wonder if we won't break anyone with this change. Technically, nobody should be using these dependencies, but you never know.

@Zirak
Copy link
Contributor Author

Zirak commented Apr 2, 2024

Thanks!

Technically, nobody should be using these dependencies, but you never know.

Interesting! From my understanding the only possible user for these dependencies is the nodejs agent itself. Since the modules end up in /autoinstrumentation/node_modules, an end user shouldn't really be exposed to them.

@pavolloffay pavolloffay merged commit aeb19a7 into open-telemetry:main Apr 2, 2024
32 checks passed
@Zirak Zirak deleted the js-build-remove-dev branch April 2, 2024 19:18
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…elemetry#2803)

The autoinstrumentation has several build-time dependencies which aren't
necessary for the final image, like typescript. Comparison of builds without
typescript, typescript + rimraf, and finally all dev dependencies:

```
$ docker image ls --format 'table {{.Repository}} {{.Tag}} {{.Size}}'
autoinstrumentation-nodejs 60d37e1 272MB
autoinstrumentation-nodejs no-ts 208MB
autoinstrumentation-nodejs no-ts-rmrf 201MB
autoinstrumentation-nodejs no-dev 200MB
```

We extend the existing `postcompile` script. After building into
`build/workspace`, we `prune` to remove extraneous packages. Doing that requires
the `package.json` in the build directory.
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.

Node autoinstrumentation image includes dev dependencies
3 participants