Initial Twirp Ruby instrumentation #1747
Conversation
|
|
Tonkpils
force-pushed
the
tonkpils/twirp
branch
from
df4154f to
43047f5
Compare
thompson-tomo
left a comment
thompson-tomo
left a comment
There was a problem hiding this comment.
The RPC conventions are currently ungergoing a stabilisation effort which will introduce breaking changes ie rpc.system will be renamed. As such I would suggest waiting for those breaking changes before merging this. I have raised an issue to add twirp open-telemetry/semantic-conventions#2948. Note the changes suggested have already been reviewed as part of that stabilisation effort.
| OpenTelemetry::SemanticConventions::Trace::RPC_SYSTEM => 'twirp', | ||
| OpenTelemetry::SemanticConventions::Trace::RPC_SERVICE => service_name, | ||
| OpenTelemetry::SemanticConventions::Trace::RPC_METHOD => method_name, | ||
| 'rpc.twirp.content_type' => @content_type |
There was a problem hiding this comment.
Could the http headers attribute be used?
| attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME] = url_prefix.host if url_prefix.host | ||
| attributes[OpenTelemetry::SemanticConventions::Trace::NET_PEER_PORT] = url_prefix.port if url_prefix.port |
There was a problem hiding this comment.
These should be network instead of net.
| method_name = rpc_method.to_s | ||
|
|
||
| attributes = { | ||
| OpenTelemetry::SemanticConventions::Trace::RPC_SYSTEM => 'twirp', |
There was a problem hiding this comment.
Can we add the server.address/port attributes?
| method_name = rpc_method.to_s | ||
|
|
||
| attributes = { | ||
| OpenTelemetry::SemanticConventions::Trace::RPC_SYSTEM => 'twirp', |
There was a problem hiding this comment.
Can we add the network.protocol.name/version attributes?
| method_name = rpc_method.to_s | ||
|
|
||
| attributes = { | ||
| OpenTelemetry::SemanticConventions::Trace::RPC_SYSTEM => 'twirp', |
There was a problem hiding this comment.
Can we add the network.transport attribute?
| if response.error | ||
| error = response.error | ||
| span.set_attribute('rpc.twirp.error_code', error.code.to_s) if error.code | ||
| span.set_attribute('rpc.twirp.error_msg', error.msg) if error.msg |
There was a problem hiding this comment.
Do we really need this given that we have the span status description?
| # Handle response | ||
| if response.error | ||
| error = response.error | ||
| span.set_attribute('rpc.twirp.error_code', error.code.to_s) if error.code |
There was a problem hiding this comment.
Remove rpc from attribute name.
| if rpc_method && !rpc_method.empty? | ||
| # Set RPC semantic attributes | ||
| span.set_attribute(OpenTelemetry::SemanticConventions::Trace::RPC_SYSTEM, 'twirp') | ||
| span.set_attribute(OpenTelemetry::SemanticConventions::Trace::RPC_SERVICE, service_name) | ||
| span.set_attribute(OpenTelemetry::SemanticConventions::Trace::RPC_METHOD, rpc_method) | ||
|
|
||
| # Update span name to RPC method | ||
| span.name = "#{service_name}/#{rpc_method}" | ||
| end | ||
|
|
||
| # Add content type if available | ||
| content_type = rack_env['CONTENT_TYPE'] | ||
| span.set_attribute('rpc.twirp.content_type', content_type) if content_type |
|
@thompson-tomo one of the challenges with Twirp is that it is a HTTP based RPC protocol. There are multiple layers of client libraries similar to the issue described here: open-telemetry/semantic-conventions#674 We (GitHub) are also still using pre-1.0 conventions; so for the purposes of this implementation we will want to implement OPT_IN compatibility for at least the initial release. |
|
@arielvalentin i don't see twirp being any different to connect-rpc which can/does also operate over http in a similar manner. Would having the opt in capability focus on the net vs network attributes but use the rpc.system replacement attribute from the initial release. |
|
Yes that would work use case. The part I'd like to resolve is the modeling of client spans. Twirp uses HTTP/1.1 via Faraday to make http requests. What we currently do by default with the Faraday instrumentation is enrich the driver span with additional attributes; that way the client span has additional contextual attributes about the request that the driver may not know about. Rather than having multiple client spans or a dedicated twirp span, I thought about suggesting the approach of enriching the HTTP client span with Twirp-RPC semantics attributes. The change we would need to introduce to http client instrumentations is a name formatter that we could inject using an existing semantic attribute. The other option would be to do what the AWS SDK does, which is to disable "internal instrumentation. I find this approach undesirable for our use case because we are keen on seeing details about our http drivers that are abstracted away from the twirp library itself E.g. TCP connect spans. Any concerns? |
|
I think, I know what you are thinking of and I have similiar thought. I am also not a fan of disabling the internal instrumentation. The way I see it is when a RPC method is invoked by a client, a span is created. That span would folow conventions based on the protocol which is either an extension of another protocol ie http or of the base rpc span conventions. I certainly wouldn't want multiple logical operation spans and i would much prefer to see http as a seamless part of rpc conventions which means on the same span. Note this is part of the reason why I included http as a protocol in open-telemetry/semantic-conventions#2842 to potentially bring them together etc. In terms of how best to achieve this, is hard to say but I would see a simple solution being advantageous. Hope that sort of answered your question. |
|
@thompson-tomo we have a
Thank you for the contribution to semantic conventions! I am curious what others will think about the framework concept. We have an existing helper function that adds context to HTTP client spans, which we later use to enrich the driver spans: https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/old/client.rb#L31 For HTTP Semantics the Driver span name formatter can use attributes like I am not sure there is any value at the moment to have a dedicated span for the Twirp framework itself, except for representing the time taken in the framework code and to have a specific instrumentation scope visible to our users. I am inclined to omit it for now and only use attribute enrichment for the client side instrumentation. |
No worries, getting feedback seems to be the hardest parts. 🤣
Am aware and there is an issue to update the span naming for rpc. The guidance for http span names won't change.
Agree benefit is limited especially if the only thing the framework is doing other than packaging the data is making a single http request. |
|
After chatting with @arielvalentin, I'm going to go ahead and close this out to explore a different and more scoped approach. I'll likely be focusing on the twirp client instrumentation first and exploring potentially pushing the attributes so that the driver (e.g Faraday) can use expand those. Thank you all for the feedback here 🙇 |
3 participants
No description provided.