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(tracing): open-telemetry traces #1039

Merged
merged 1 commit into from
Dec 7, 2024
Merged

Conversation

seriousben
Copy link
Member

@seriousben seriousben commented Nov 19, 2024

What

This PR enables exporting opentelemetry traces to a given endpoint. The endpoint can be either configured in the config file or using the standard opentelemetry environment variables.

config.yml example:

tracing:
  enabled: true

Changes:

  • Config change to support enabling tracing
  • Change to how the config parsing work to merge config values with the default

A look at existing traces

As we start using traces more and more, we'll need to format and add new traces as needed.

image

Tracing propagation

We are using W3C Tracecontext to propagate traces. It reads a parent trace-id from the traceparent header and returns a new trace + span back in the response headers.

curl -v -H "traceparent: 00-d5fe1dc9035165ce36952daf29686b6c-14330be33197dd1a-01" http://localhost:8900/namespaces
* Host localhost:8900 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8900...
* connect to ::1 port 8900 from ::1 port 53044 failed: Connection refused
*   Trying 127.0.0.1:8900...
* Connected to localhost (127.0.0.1) port 8900
> GET /namespaces HTTP/1.1
> Host: localhost:8900
> User-Agent: curl/8.7.1
> Accept: */*
> traceparent: 00-d5fe1dc9035165ce36952daf29686b6c-14330be33197dd1a-01
>
* Request completely sent off
< HTTP/1.1 200 OK
< content-type: application/json
< traceparent: 00-d5fe1dc9035165ce36952daf29686b6c-574acef8aad9a667-01
< tracestate:
< vary: origin, access-control-request-method, access-control-request-headers
< access-control-allow-origin: *
< content-length: 62
< date: Wed, 04 Dec 2024 20:21:45 GMT
<
* Connection #0 to host localhost left intact
{"namespaces":[{"name":"default","created_at":1733342892666}]}

Testing

Run server with the config:

tracing:
  enabled: true
run --rm --name jaeger \
                                        -e COLLECTOR_OTLP_ENABLED:true \
                                        -p 16686:16686 \
                                        -p 4317:4317 \
                                        -p 4318:4318 \
                                       jaegertracing/all-in-one:1.49

Open http://localhost:16686

Contribution Checklist

  • If the python-sdk was changed, please run make fmt in python-sdk/.
  • If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

@seriousben
Copy link
Member Author

This needs better integration with axum's traces, especially with regards to error results before being ready.

It also needs configuration to toggle tracing export.

Otherwise, this PR works well with Jaeger.

@diptanu
Copy link
Collaborator

diptanu commented Dec 1, 2024

Whats left here?

@seriousben seriousben force-pushed the seriousben/support-tracing branch 2 times, most recently from b5ee985 to bc44032 Compare December 4, 2024 02:04
@seriousben
Copy link
Member Author

This now only needs more spans and more instrument in the code as well as a configuration toggle for tracing.

@seriousben seriousben force-pushed the seriousben/support-tracing branch 3 times, most recently from 1fe63ac to 462a56e Compare December 4, 2024 20:22
"Failed for node: {}",
node
);
let mut parent_nodes = graph.get_compute_parent_nodes(node);
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated change triggered by clippy.

@seriousben seriousben changed the title WIP open-telemetry traces feattracing(): open-telemetry traces Dec 4, 2024
@seriousben seriousben changed the title feattracing(): open-telemetry traces feat(tracing): open-telemetry traces Dec 4, 2024
@seriousben seriousben requested a review from diptanu December 5, 2024 19:29
@seriousben seriousben force-pushed the seriousben/support-tracing branch from 462a56e to f80cbbc Compare December 7, 2024 12:33
@seriousben seriousben merged commit 7e188a6 into main Dec 7, 2024
5 checks passed
@seriousben seriousben deleted the seriousben/support-tracing branch December 7, 2024 12:55
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