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: add Elastic OpenTelemetry Node.js distribution #21

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

rogercoll
Copy link
Collaborator

The payment service has been modified to use the Elastic's OpenTelemetry Distribution for Node.js instead of the upstream version.

Changes

Please provide a brief description of the changes here.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

* [ ] CHANGELOG.md updated to document new feature additions
* [ ] Appropriate documentation updates in the docs
* [ ] Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@@ -7,27 +7,28 @@
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"lint": "semistandard *.js",
"start": "node --require ./opentelemetry.js index.js"
"start": "OTEL_EXPORTER_OTLP_PROTOCOL=grpc node --require @elastic/opentelemetry-node index.js"
Copy link
Member

Choose a reason for hiding this comment

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

do you plant to remove opentelemetry.js file? If so there are several dependencies that are recommended to delete to avoid version conflicts with dependencies declared in @elastic/opentelemetry-node. In fact they could be deleted and opentelemetry.js would still work.

The list of deps becomes the one below

{
  "name": "paymentservice",
  ...
  "dependencies": {
    "@elastic/opentelemetry-node": "^0.2.0",
    "@grpc/grpc-js": "1.10.8",
    "@grpc/proto-loader": "0.7.13",
    "@openfeature/flagd-provider": "0.13.0",
    "@openfeature/server-sdk": "1.14.0",
    "@opentelemetry/api": "1.8.0",
    "@opentelemetry/auto-instrumentations-node": "0.46.1",
    "grpc-js-health-check": "1.1.0",
    "pino": "8.16.1",
    "simple-card-validator": "1.1.0",
    "uuid": "9.0.1"
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Let's remove it so the difference is more visible too. Fixed in f5a79b4

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Note that I only really reviewed the src/paymentservice/... changes.

Copy link
Member

Choose a reason for hiding this comment

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

This also includes a bunch of changes that aren't directly about switching to the Elastic Node.js distro. Is that intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I guess those changes were shown due to the automatic main branch rebase. 😞

Branch rebased.

"@opentelemetry/resource-detector-aws": "1.5.0",
"@opentelemetry/resource-detector-container": "0.3.9",
"@opentelemetry/resource-detector-gcp": "0.29.9",
"@opentelemetry/auto-instrumentations-node": "0.46.1",
Copy link
Member

Choose a reason for hiding this comment

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

This dep could also be removed. The @elastic/opentelemetry-node includes any instrumentations that it supports as dependencies itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this! Removed in 72a8b3c

The payment service has been modified to use the Elastic's OpenTelemetry
Distribution for Node.js instead of the upstream version.
@rogercoll rogercoll merged commit d0f2803 into elastic:main Jun 19, 2024
4 checks passed
elastic-apm-tech pushed a commit that referenced this pull request Jun 19, 2024
* feat: add Elastic OpenTelemetry Node.js distribution

The payment service has been modified to use the Elastic's OpenTelemetry
Distribution for Node.js instead of the upstream version.

* remove opentelemetry.js file and dependencies

* remove auto instrumentation dependency
@rogercoll rogercoll deleted the add_node_sdk branch August 27, 2024 07:12
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