feat: Add Rage instrumentation#1904
Conversation
|
Thank you @rsamoilov for your contribution. I see that you are a core contributor of Our preference would eventually be for library maintainers to maintain first party instrumentations in their respective repositories as opposed to the contrib package. In other words, have the This would give you the flexibility to maintain the instrumentation as a part of your organization and reduce friction trying to get changes merged into the contrib repo. We have limited resources and lack the expertise in the framework so it will be difficult for us to provide you with a thorough review. The areas where I think that we will be able to help with code-reviews or unblock you when faced with incompatibilities with the SDK. Is this something you would be amenable to? |
|
This makes sense to me. I will create a repo and get back to you. Thanks! |
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/cable.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/cable.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/cable.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/deferred.rb
Outdated
Show resolved
Hide resolved
|
|
||
| OpenTelemetry::Context.with_current(otel_context) do | ||
| attributes = { | ||
| SemConv::Incubating::MESSAGING::MESSAGING_SYSTEM => 'rage.deferred', |
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/events.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/request.rb
Outdated
Show resolved
Hide resolved
| http_route = request.route_uri_pattern | ||
| span.name = "#{request.method} #{http_route}" | ||
|
|
||
| attributes = { |
There was a problem hiding this comment.
Is there any additional semconv attributes that could be added?
There was a problem hiding this comment.
Not that I can think of - these are pretty much the same attributes Action Pack adds to requests. Any recommendations?
There was a problem hiding this comment.
There was a problem hiding this comment.
You're referring to a client span for outbound connections. handlers/request.rb processes inbound connections.
There was a problem hiding this comment.
In that case look at the server span.
There was a problem hiding this comment.
Most of those attribute are already being added to the span inside the Rack instrumentation:
http.request.methodserver.addressurl.schemeurl.pathurl.queryuser_agent.originalhttp.routehttp.response.status_code
There was a problem hiding this comment.
Are you saying that those attributes are being added to this span elsewhere or are they on different spans?
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/deferred.rb
Outdated
Show resolved
Hide resolved
|
@arielvalentin I've created rage-rb/opentelemetry-instrumentation. Let me know how you'd like to proceed. |
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/cable.rb
Outdated
Show resolved
Hide resolved
| OpenTelemetry::Context.with_current(handshake_context) do | ||
| attributes = { | ||
| SemConv::Incubating::MESSAGING::MESSAGING_SYSTEM => 'rage.cable', | ||
| SemConv::Incubating::MESSAGING::MESSAGING_DESTINATION_NAME => channel.class.name, |
There was a problem hiding this comment.
| SemConv::Incubating::MESSAGING::MESSAGING_DESTINATION_NAME => channel.class.name, | |
| 'rage.cable.stream.name' => connection.class.name, |
| attributes = { | ||
| SemConv::Incubating::MESSAGING::MESSAGING_SYSTEM => 'rage.cable', | ||
| SemConv::Incubating::MESSAGING::MESSAGING_DESTINATION_NAME => connection.class.name, | ||
| SemConv::Incubating::CODE::CODE_FUNCTION_NAME => "#{connection.class}##{action}" |
There was a problem hiding this comment.
| SemConv::Incubating::CODE::CODE_FUNCTION_NAME => "#{connection.class}##{action}" | |
| 'rpc.operation.type' => "{action}" |
There was a problem hiding this comment.
Cable is a pub/sub (broadcast to subscribers) system, not RPC (call and wait for response). While channels can be triggered usin RPC actions, there's no return value. Using rpc.operation.type essentially misclassifies the system.
RPC also considers streaming which is not request and response.
There was a problem hiding this comment.
It's a bit hard for me to reason about it without understanding what a messaging system is in the OpenTelemetry terms.
Anyway, the latest semantic conventions state that for streaming RPCs, the server span should cover the full lifetime of the request and/or response streams until they are closed or terminated.
This implies that:
- RPC is considered request/response, which is not true for WebSockets.
- The RPC span should cover the entire lifetime of the WebSocket connection. This is unrealistic for WebSockets as connections can be open for days and weeks.
Please let me know if I've misinterpreted it.
There was a problem hiding this comment.
I agree that the defination of rpc covering the entire duration as one span makes it harder, hopefully it will be addressed as part of the rpc stabilisation effort. I do know streaming was initially out of scope.
It's a bit hard for me to reason about it without understanding what a messaging system is in the OpenTelemetry terms.
Agree that having the definition/requirements of things like messaging.system documented is important.
There was a problem hiding this comment.
But you do recommend going with the RPC attributes, right?
| attributes = { | ||
| SemConv::Incubating::MESSAGING::MESSAGING_SYSTEM => 'rage.cable', | ||
| SemConv::Incubating::MESSAGING::MESSAGING_DESTINATION_NAME => channel.class.name, | ||
| SemConv::Incubating::CODE::CODE_FUNCTION_NAME => "#{channel.class}##{action}" |
There was a problem hiding this comment.
| SemConv::Incubating::CODE::CODE_FUNCTION_NAME => "#{channel.class}##{action}" | |
| 'rpc.operation.type' => "{action}" |
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/cable.rb
Show resolved
Hide resolved
| def self.create_broadcast_span(stream:) | ||
| attributes = { | ||
| SemConv::Incubating::MESSAGING::MESSAGING_SYSTEM => 'rage.cable', | ||
| SemConv::Incubating::MESSAGING::MESSAGING_OPERATION_TYPE => 'publish', |
There was a problem hiding this comment.
| SemConv::Incubating::MESSAGING::MESSAGING_OPERATION_TYPE => 'publish', | |
| 'rpc.operation.type' => 'publish', |
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/deferred.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/events.rb
Outdated
Show resolved
Hide resolved
instrumentation/rage/lib/opentelemetry/instrumentation/rage/handlers/events.rb
Outdated
Show resolved
Hide resolved
| http_route = request.route_uri_pattern | ||
| span.name = "#{request.method} #{http_route}" | ||
|
|
||
| attributes = { |
There was a problem hiding this comment.
In that case look at the server span.
|
Hey @thompson-tomo , thanks for the review!
Same as with Action Cable, streams aren't properties of connections - they are being created when the application code executes Using
Looking at other implementations and OTel docs,
Cable is a pub/sub (broadcast to subscribers) system, not RPC (call and wait for response). While channels can be triggered usin RPC actions, there's no return value. Using
Background job queues have all attributes of messaging systems; since workflow conventions are still a proposal, I'd propose using established messaging conventions now and migrating to |
|
Have gone and transfered the above feedback to the review comments and responded accordingly |
|
@arielvalentin @thompson-tomo Just checking in on the PR - please let me know how we can proceed. Currently, most disagreements are about the Cable instrumentation. I’m happy to delete it and tackle in another PR if you think it makes sense. |
|
@arielvalentin @thompson-tomo Hey folks, thank you for your feedback! I'll give the PR one more week for review. If there are no concerns, I'll then move forward with maintaining this under the Rage org. @dazuma Would you be able to take a look at the instrumentation when you get a chance? |
|
@rsamoilov please proceed with releasing the code as a part of your repo! Happy to help answer questions or provide feedback there as well. |
the "what code rag" info is carried inside the span name; `code.*` is used for debugging, not primary semantic modeling;
|
@thompson-tomo Made some updates:
|
|
Closed in favor of #1975. |
Summary
This PR adds OpenTelemetry instrumentation for Rage, a fiber-based Ruby web framework.
The instrumentation leverages Rage's built-in Telemetry interface to instrument:
Implementation Details:
Note: The Rage version compatible with this instrumentation (1.20.0+) has not been released yet. This PR is intended to be merged before the Rage release so that the instrumentation is available immediately when users upgrade.