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

[Cargo] Update opentelemetry crates to their latest version #92

Merged
merged 4 commits into from
Jul 27, 2024

Conversation

jaudiger
Copy link
Contributor

@jaudiger jaudiger commented Jul 16, 2024

Update all the opentelemetry crates. opentelemetry-jaeger dependency was deprecated in favor of opentelemetry-otlp which provides agnostic APIs to export traces, logs, metrics.

Unfortunately, due to console-subscriber we now have to duplicate some dependencies such as tonic...

It depends on the unreleased version 0.25.0 of tracing-opentelemetry, see PR (for context: tokio-rs/tracing-opentelemetry#155)

@jaudiger jaudiger marked this pull request as draft July 16, 2024 15:57
@jaudiger jaudiger changed the title [Cargo] Update opentelemetry crates to their latest versions [Cargo] Update opentelemetry crates to their latest version Jul 16, 2024
@kylewlacy
Copy link
Member

Update all the opentelemetry crates. opentelemetry-jaeger dependency was deprecated in favor of opentelemetry-otlp which provides agnostic APIs to export traces, logs, metrics

I think it was already deprecated when I first set it up too 😅 I just couldn't figure out how to get opentelemetry-otlp wired up to talk to Jaeger properly, so I'll have to try this out locally to see if I can get it working...

@jaudiger
Copy link
Contributor Author

I think it was already deprecated when I first set it up too 😅 I just couldn't figure out how to get opentelemetry-otlp wired up to talk to Jaeger properly, so I'll have to try this out locally to see if I can get it working...

I could set up Jaeger locally if you want and test all of that, I'm familiar with this tool.

@jaudiger jaudiger force-pushed the update-otel-crates branch 2 times, most recently from 3401bf4 to e799597 Compare July 22, 2024 05:28
Signed-off-by: jaudiger <jeremy.audiger@ioterop.com>
@jaudiger jaudiger force-pushed the update-otel-crates branch from e799597 to 7ac0709 Compare July 22, 2024 07:24
jaudiger added 2 commits July 23, 2024 22:17
Signed-off-by: jaudiger <jeremy.audiger@ioterop.com>
Signed-off-by: jaudiger <jeremy.audiger@ioterop.com>
@jaudiger jaudiger marked this pull request as ready for review July 25, 2024 06:31
@jaudiger
Copy link
Contributor Author

I finished testing this PR. And the reporting of traces is not broken, but an adaptation could be needed depending on how you're testing before @kylewlacy.

I used the docker-compose.yml below to deploy Jaeger, due to a limitation with the latest version 1.59, a temporary env var needs to be defined in some cases depending on mainly how all the things are deployed, and exposed:

The OTEL Collector upgrade brought in a change where OTLP receivers started listening on localhost instead of 0.0.0.0 as before. As a result, when running in container environment the endpoints are likely unreachable from other containers (Issue jaegertracing/jaeger#5737). The fix will be available in the next release. Meanwhile, the workaround is to instruct Jaeger to listen on 0.0.0.0, as in this fix:

services:
  jaeger:
    image: docker.io/bitnami/jaeger:1.59.0
    ports:
      - 6831:6831/udp
      - 6832:6832/udp
      - 5778:5778
      - 16686:16686
      - 4317:4317
      - 4318:4318
      - 14250:14250
      - 14268:14268
      - 14269:14269
      - 9411:9411
    environment:
      - COLLECTOR_ZIPKIN_HOST_PORT=:9411
      - COLLECTOR_OTLP_ENABLED=true
      - COLLECTOR_OTLP_GRPC_HOST_PORT=0.0.0.0:4317
      - COLLECTOR_OTLP_HTTP_HOST_PORT=0.0.0.0:4318
      - LOG_LEVEL=debug
      - JAEGER_USERNAME=
      - JAEGER_PASSWORD=

Brioche was launched inside another docker container with the following command:

docker run --name brioche-devenv --platform linux/amd64 -v $(pwd):/workspace -v $(pwd)/../brioche-packages:/tmp/brioche-packages -v brioche-cache:/home/container/.local/share/brioche -it --link jaeger-jaeger-1 --net jaeger_default brioche bash

The link and network arguments are here necessary, since I launched Jaeger through docker compose, and a new docker network was created by the compose command. I could have put everything inside the same docker-compose.yml file, though.

And then inside the container:

BRIOCHE_JAEGER_ENDPOINT=http://jaeger:4318/v1/traces cargo run -- fmt -p /tmp/brioche-packages/packages/bat

image

The difference between main and this branch is the endpoint used. On main, we need to pass jaeger: 6831 whereas now it's http://jaeger:4318/v1/traces.

The port 4318 accepts traces in OpenTelemetry OTLP format .

I could have used the port 4317, but I had issues with gRPC. The traces weren't exported correctly to Jaeger, only the child spans were exported, not the parent ones. The traces were incomplete. I suspect an issue inside one of the otel libraries used.

Copy link
Member

@kylewlacy kylewlacy left a comment

Choose a reason for hiding this comment

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

Tested this out and works great, LGTM!

@kylewlacy
Copy link
Member

Oh and it might be worth changing the env vars at some point since this is no longer Jaeger-specific, but since this is more of an internal debugging tool I don't feel strongly about when that happens. I might take it on sometime when doing some code cleanup

@kylewlacy kylewlacy enabled auto-merge (squash) July 27, 2024 07:09
@kylewlacy kylewlacy merged commit 9c0cb4f into brioche-dev:main Jul 27, 2024
5 checks passed
@jaudiger jaudiger deleted the update-otel-crates branch July 29, 2024 07:18
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.

2 participants