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

Add Node.js example #284

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

GhassenRjab
Copy link

With this PR I add example app for Node.js.

The example is based on the Node.js app written in OpenTelemetry documentation (dice server)

Even though the OpenTelemetry lib for Logging in Node.js is still in development I use it in the example. It is working well

@CLAassistant
Copy link

CLAassistant commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

@GhassenRjab GhassenRjab force-pushed the nodejs-example branch 3 times, most recently from 05e7271 to a5158d4 Compare February 13, 2025 05:58
@zeitlinger
Copy link
Member

Thanks a lot for the contribution!

We try to make the simplest example possible, therefore: Have you considered using zero-code? https://opentelemetry.io/docs/zero-code/js/

@GhassenRjab
Copy link
Author

Thank you for your feedback and the recommendation @zeitlinger!

I didn't know about the zero-code feature before.
As I was reading through its documentation, I saw it only support traces.
It means that I'll have to configure metrics and logs anyway.
I think it's not worth it. Can we keep the current implementation?

Also, I managed to reproduce the lint issues locally, and fixed them.

As of the OATS, I was unable to reproduce locally.
I made some changes and I hope they fix it.
But if they don't, can you guide me how to run/reproduce locally?

@zeitlinger zeitlinger enabled auto-merge (squash) February 14, 2025 13:49
@zeitlinger
Copy link
Member

Thank you for your feedback and the recommendation @zeitlinger!

I didn't know about the zero-code feature before. As I was reading through its documentation, I saw it only support traces. It means that I'll have to configure metrics and logs anyway. I think it's not worth it. Can we keep the current implementation?

... and I wasn't aware that the auto-instrumentation has this limitation 😄

Agree - let's keep this and maybe add an auto-instrumentation example later.

Also, I managed to reproduce the lint issues locally, and fixed them.

great

As of the OATS, I was unable to reproduce locally. I made some changes and I hope they fix it. But if they don't, can you guide me how to run/reproduce locally?

did you see https://github.com/grafana/docker-otel-lgtm/blob/main/CONTRIBUTING.md ?

@zeitlinger zeitlinger disabled auto-merge February 14, 2025 13:58
@zeitlinger zeitlinger enabled auto-merge (squash) February 14, 2025 14:01
@zeitlinger
Copy link
Member

As of the OATS, I was unable to reproduce locally. I made some changes and I hope they fix it. But if they don't, can you guide me how to run/reproduce locally?

did you see https://github.com/grafana/docker-otel-lgtm/blob/main/CONTRIBUTING.md ?

I already saw a problem with a port - let's see if that fixes it

Copy link
Member

@zeitlinger zeitlinger left a comment

Choose a reason for hiding this comment

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

I'm thinking that you need the protobuf port based on the dependencies

environment:
OTEL_SERVICE_NAME: "dice-server"
OTEL_SERVICE_VERSION: "0.1.0"
OTEL_EXPORTER_OTLP_ENDPOINT: http://lgtm:4317
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
OTEL_EXPORTER_OTLP_ENDPOINT: http://lgtm:4317
OTEL_EXPORTER_OTLP_ENDPOINT: http://lgtm:4318

auto-merge was automatically disabled February 14, 2025 20:29

Head branch was pushed to by a user without write access

- Lint again files and disable javascript standard. It had a conflict with Prettier about this rule space-before-function-paren
- Use proto port
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.

3 participants