-
Notifications
You must be signed in to change notification settings - Fork 166
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 SetRequestId
and PropagateRequestId
middleware
#150
Conversation
bdfd1e3
to
454c838
Compare
454c838
to
d3e9d3f
Compare
SetRequestId
middlewareSetRequestId
and PropagateRequestId
middleware
@DoumanAsh what do you think about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks pretty good and generic too for most use cases
I left some comments based on my own middleware
OpenTelemetry defines a bunch of [conventions for HTTP spans][conventions]. This makes adopting those conventions easy when using `Trace`: ```rust .layer(TraceLayer::new_for_http().opentelemetry_server()) ``` This changes the span for the request to: - Name: HTTP request - Fields: - `http.flavor`: "1.1" - `http.host`: "localhost:3000" - `http.method`: "GET" - `http.route`: "/users/123". Can be configured by user to instead be the path pattern for the route, ie `/users/:id` - `http.scheme`: "http". Must be set by user as middleware don't know this - `http.target`: "/users/123" - `http.user_agent`: "curl/7.64.1" - `http.client_ip`: Must be extracted by user in a "make service" - `http.status_code`: 200 - `request_id`: Requires #150. - `otel.kind`: "server" - `otel.status_code`: Unset or "ERROR" - `exception.message`: Set by user based on response classification. - `exception.details`: Set by user based on response classification. The fields are a bit different for HTTP clients hence the name `opentelemetry_server`. I don't think supporting clients initially is required. We can always add it later. TODO - [ ] Add `Trace::opentelemetry_server` method. Currently it only exists on the layer - [ ] Docs - [ ] Example using axum and `opentelemetry_jaeger` - [ ] Changelog [conventions]: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this feature! Will be super nice when its merged, I only had a couple of small comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, convenient implementation
Fixes #135
Example usage:
Or slightly shorter using
ServiceBuilderExt
:These are two different middleware since they need to be applied around any logging you might have. The docs have an example.