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

fix: Backport Rack proxy event to middleware #764

Merged

Conversation

fbogsany
Copy link
Contributor

The Rack Event handler instrumentation adds a Span Event representing the time the request was enqueued. See

request_start_time = OpenTelemetry::Instrumentation::Rack::Util::QueueTime.get_request_start(request.env)
span.add_event('http.proxy.request.started', timestamp: request_start_time) unless request_start_time.nil?

The older middleware-based instrumentation represents the same information (optionally enabled via config[:record_frontend_span]) as a http_server.proxy span as a "fake" parent span for the Rack server span. The Span Event approach is both more correct and more useful for analysis.

This PR "backports" the Span Event approach to the middleware instrumentation. Since this is done unconditionally in the Rack Event handler instrumentation, I've made it unconditional in the middleware as well. If the config[:record_frontend_span] option is enabled, this will duplicate information, but I would argue we should deprecate that config option (and the corresponding code).

Many thanks to @arielvalentin for writing great code that I can steal to inspire me.

@arielvalentin arielvalentin merged commit 3d0f818 into main Dec 15, 2023
49 checks passed
@arielvalentin arielvalentin deleted the backport_rack_proxy_event_to_middleware_instrumentation branch December 15, 2023 23:09
@github-actions github-actions bot mentioned this pull request Dec 22, 2023
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