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

chore: Update the express example #2532

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

Conversation

sp6370
Copy link

@sp6370 sp6370 commented Nov 16, 2024

Which problem is this PR solving?

Short description of the changes

  • Nukes the Zipkin usage and usage of Jaeger Otel Library
  • Added a fix for the code compilation failing
  • Update the otel dependencies to latest version
  • Also fix the deprecations for the semantic conventions

@sp6370 sp6370 requested a review from a team as a code owner November 16, 2024 00:42
@sp6370
Copy link
Author

sp6370 commented Nov 16, 2024

@trentm Thanks a lot for your help and feedback on the PR. Looking forward to the code review! 😄

@sp6370 sp6370 changed the title Updates to the express example. chore: Updates to the express example Nov 16, 2024
@sp6370 sp6370 changed the title chore: Updates to the express example chore: Update the express example Nov 16, 2024
Copy link
Contributor

@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.

Some notes. Looking good, thanks.

@@ -1,6 +1,6 @@
# Overview

OpenTelemetry Express Instrumentation allows the user to automatically collect trace data and export them to the backend of choice (we can use Zipkin or Jaeger for this example), to give observability to distributed systems.
OpenTelemetry Express Instrumentation allows the user to automatically collect trace data and export them to the backend of choice (we can Jaeger for this example), to give observability to distributed systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OpenTelemetry Express Instrumentation allows the user to automatically collect trace data and export them to the backend of choice (we can Jaeger for this example), to give observability to distributed systems.
OpenTelemetry Express Instrumentation allows the user to automatically collect trace data and export them to the backend of choice (we use Jaeger for this example), to give observability to distributed systems.

Comment on lines 24 to 26
### Jaeger

Run the server:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Jaeger
Run the server:
Run the server:

Now that the example only uses Jaeger, I think the subsection title can be dropped.

Comment on lines 7 to 8
"jaeger:server": "cross-env EXPORTER=jaeger ts-node src/server.ts",
"jaeger:client": "cross-env EXPORTER=jaeger ts-node src/client.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about dropping the jaeger: prefix on these now that there is just the one target? I think it is less confusing to the user to npm run server.

Suggested change
"jaeger:server": "cross-env EXPORTER=jaeger ts-node src/server.ts",
"jaeger:client": "cross-env EXPORTER=jaeger ts-node src/client.ts",
"server": "ts-node src/server.ts",
"client": "ts-node src/client.ts",

The EXPORTER envvar is no longer necessary, and then cross-env is no longer necessary.

If you agree, please also:

  • run: npm uninstall cross-env
  • update the npm run ... commands in README.md


const Exporter = (process.env.EXPORTER || '').toLowerCase().startsWith('z') ? ZipkinExporter : OTLPTraceExporter;
const Exporter = OTLPTraceExporter;
Copy link
Contributor

Choose a reason for hiding this comment

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

This coiuld be simplified to just use OTLPTraceExporter below.

Suggested change
const Exporter = OTLPTraceExporter;

Comment on lines +38 to 39
const exporter = new Exporter({});

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const exporter = new Exporter({});
const exporter = new OTLPTraceExporter();

Setup [Zipkin Tracing](https://zipkin.io/pages/quickstart.html)
or
Setup [Jaeger Tracing](https://www.jaegertracing.io/docs/latest/getting-started/#all-in-one)
Setup [Jaeger Tracing](https://www.jaegertracing.io/docs/2.0/getting-started/#in-docker)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be okay to repeat the one-liner to start Jaeger. Something like:

Suggested change
Setup [Jaeger Tracing](https://www.jaegertracing.io/docs/2.0/getting-started/#in-docker)
Start Jaeger in Docker for receiving tracing data (see [the Jaeger docs](https://www.jaegertracing.io/docs/2.0/getting-started/#in-docker) for more details in running Jaeger):
```bash
docker run --rm --name jaeger \
-p 5778:5778 \
-p 16686:16686 \
-p 4317:4317 \
-p 4318:4318 \
-p 14250:14250 \
-p 14268:14268 \
-p 9411:9411 \
jaegertracing/jaeger:2.0.0 \
--set receivers.otlp.protocols.http.endpoint=0.0.0.0:4318 \
--set receivers.otlp.protocols.grpc.endpoint=0.0.0.0:4317
```

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.75%. Comparing base (5eb61d8) to head (0baefbc).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2532   +/-   ##
=======================================
  Coverage   90.75%   90.75%           
=======================================
  Files         169      169           
  Lines        8018     8018           
  Branches     1632     1632           
=======================================
  Hits         7277     7277           
  Misses        741      741           
---- 🚨 Try these New Features:

@pichlermarc pichlermarc added pkg:instrumentation-express documentation Improvements or additions to documentation labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pkg:instrumentation-express
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants