Skip to content

chore: Updated Node Agent init container#88

Draft
mrickard wants to merge 3 commits into
newrelic-experimental:mainfrom
mrickard:NR-260174/node-agent
Draft

chore: Updated Node Agent init container#88
mrickard wants to merge 3 commits into
newrelic-experimental:mainfrom
mrickard:NR-260174/node-agent

Conversation

@mrickard

@mrickard mrickard commented May 14, 2024

Copy link
Copy Markdown

This PR adds build args to the Dockerfile for Node, letting us target agent versions and runtimes via GHA matrices. (Creating this workflow is a next step.)

This removes TypeScript from the Node directory--the agent isn't written in TypeScript, and does not need a build step.

TODO: update go script, update GHA workflow

mrickard added 2 commits May 14, 2024 14:48
Signed-off-by: mrickard <maurice@mauricerickard.com>
…nt version

Signed-off-by: mrickard <maurice@mauricerickard.com>
@CLAassistant

CLAassistant commented May 14, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread autoinstrumentation/nodejs/Dockerfile Outdated
# In the first stage, install all the required packages in one custom directory.
# Then in the second stage, copy the directory to `/instrumentation`.
# - Ensure you have `newrelic`, `@newrelic/aws-sdk`, `@newrelic/koa`, `@newrelic/superagent`, and `@newrelic/native-metrics`
# - Ensure you have `newrelic`, which also contains instrumentation for `aws-sdk`, `koa`, `superagent`, and `native-metrics`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@newrelic/native-metrics is still a separate dependency.

Comment thread autoinstrumentation/nodejs/Dockerfile Outdated
# - Ensure you have `newrelic`, which also contains instrumentation for `aws-sdk`, `koa`, `superagent`, and `native-metrics`.
# - Grant the necessary access to `/instrumentation` directory. `chmod -R go+r /instrumentation`
FROM node:20.12 AS build
ARG NODE_RUNTIME_VERSION=20.12

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
ARG NODE_RUNTIME_VERSION=20.12
ARG NODE_RUNTIME_VERSION=20

The list of images is at https://hub.docker.com/_/node/

Specifying a very specific release version like 20.12 is going to introduce a lot of churn in this file/repo.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I also prefer using the broader caregory of runtime version, though I wasn't sure if there was a particular requirement driving the choice of this specific one.

@@ -0,0 +1,3 @@
const newrelic = require('newrelic')

export { newrelic }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we don't have to do any exporting. this isn't being used. also this is now implying it's ESM which I wouldn't do. I think this file can just do require('newrelic')

… separate install for @newrelic/native-metrics

Signed-off-by: mrickard <maurice@mauricerickard.com>

RUN npm install
RUN npm install newrelic@${NEWRELIC_NODE_AGENT_VERSION}
RUN npm install @newrelic/native-metrics@${NEWRELIC_NODE_NATIVE_METRICS_VERSION}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the agent bundles native-metrics. you don't have to install it directly

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's listed under optionalDependencies (https://github.com/newrelic/node-newrelic/blob/0ccdc6ada757291dc020feb88bf30addb7a4f47b/package.json#L194-L214). Sorry, @mrickard, I was only looking a the dependencies block yesterday when I was verifying how it gets installed.

And it sounds like it will get installed unless we do something extra https://docs.npmjs.com/cli/v7/configuring-npm/package-json#optionaldependencies

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.

4 participants