Skip to content

Upgrade NodeJS instrumentation to v3.1.2 #1800

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

Merged
merged 11 commits into from
Apr 28, 2025
Merged

Conversation

pjanotti
Copy link
Contributor

Upgrade NodeJS instrumentation to v3.1.2 - supersedes #1744

@pjanotti pjanotti requested review from a team as code owners April 25, 2025 03:43
Copy link

mergify bot commented Apr 25, 2025

⚠️ The sha of the head commit of this PR conflicts with #1791. Mergify cannot evaluate rules on this PR. ⚠️

@@ -153,7 +153,7 @@ Helper for generating environment variables for each instrumentation library.

{{- /* Handle custom or default exporter endpoint */ -}}
{{- $customOtelExporterEndpoint := "" }}
{{- if or (eq .instLibName "dotnet") (eq .instLibName "python") (eq .instLibName "java") }}
{{- if or (eq .instLibName "dotnet") (eq .instLibName "java") (eq .instLibName "nodejs") (eq .instLibName "python") }}
{{- $customOtelExporterEndpoint = .endpoint | replace ":4317" ":4318" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should consider if we still have cases that need this conditional. In principle we could just default to 4318.

@@ -68,6 +68,12 @@ jobs:
- name: Update dependencies
run: |
make dep-update
- name: Temporary generate new nodejs image - do not upload it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently these images a pushed to quay.io in a separate step. This specific image is not that expensive to build, but, to facilitate upgrades that require changes to the test images we may want to consider saving the needed images as workflow artifacts instead of publishing them to quay.io

Copy link
Contributor

Choose a reason for hiding this comment

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

That might help. That can be tracked separately.

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

@pjanotti pjanotti merged commit 06c95f6 into main Apr 28, 2025
82 checks passed
@pjanotti pjanotti deleted the upgrade-nodejs-to-v3.1.2 branch April 28, 2025 20:35
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants