-
Notifications
You must be signed in to change notification settings - Fork 377
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 mandatory rpc and grpc tags for grpc integration #2620
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
# frozen_string_literal: true | ||
|
||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module GRPC | ||
module Formatting | ||
VALUE_UNKNOWN = 'unknown' | ||
|
||
# A class to extract GRPC span attributes from the GRPC implementing class method object. | ||
class MethodObjectFormatter | ||
# grpc_full_method is a string containing all the rpc method information (from the Protobuf definition) | ||
# in a single string with the following format: /$package.$service/$method | ||
attr_reader :grpc_full_method | ||
|
||
# legacy_grpc_service is built using the Ruby GRPC service implementation package and class name instead | ||
# of the rpc interface representation from Protobuf. It's kept for compatibility. | ||
attr_reader :legacy_grpc_service | ||
|
||
# legacy_grpc_method is built using the Ruby GRPC service implementation method name instead of the rpc | ||
# interface representation from Protobuf. It's kept for compatibility. | ||
attr_reader :legacy_grpc_method | ||
|
||
# resource_name is used for the span resource name. | ||
attr_reader :resource_name | ||
|
||
def initialize(grpc_method_object) | ||
@grpc_full_method = format_full_method(grpc_method_object) | ||
@resource_name = format_resource_name(grpc_method_object) | ||
@legacy_grpc_method = extract_legacy_grpc_method(grpc_method_object) | ||
@legacy_grpc_service = extract_legacy_grpc_service(grpc_method_object) | ||
end | ||
|
||
private | ||
|
||
def format_full_method(grpc_method_object) | ||
service = extract_grpc_service(grpc_method_object) | ||
method = extract_grpc_method(grpc_method_object) | ||
"/#{service}/#{method}" | ||
end | ||
|
||
def extract_grpc_service(grpc_method_object) | ||
owner = grpc_method_object.owner | ||
return VALUE_UNKNOWN unless owner.instance_variable_defined?(:@service_name) | ||
|
||
# Ruby protoc generated code includes this variable which directly contains the value from the original | ||
# protobuf definition | ||
owner.service_name.to_s | ||
end | ||
|
||
# extract_grpc_method attempts to find the original method name from the Protobuf file definition, | ||
# since grpc gem forces the implementation method name to be in snake_case. | ||
def extract_grpc_method(grpc_method_object) | ||
owner = grpc_method_object.owner | ||
|
||
return VALUE_UNKNOWN unless owner.instance_variable_defined?(:@rpc_descs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to invoke There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this check because for the case of the previous grpc Ruby class (manual) implementation this was not defined, and this is added by the protoc Ruby generated one. Even though I just checked the grpc gem fails to start if you provide a class without this or this having no items. Would you prefer to remove this check? (in that case I think we probably need to update some unit tests that are relying on a non-autogenerated grpc test service class if I recall correctly) |
||
|
||
method, = owner.rpc_descs.find do |k, _| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
This collection would contain all the methods (or rpcs in gRPC terminology) in that service. I can imagine most services having a magnitude of 10s or at most 100s (even though I already consider this case being weird). I guess potentially it could be anything but it doesn't seem a realistic scenario to me.
Let me try to give a little bit of extra context:
So given this situation, I don't think there's an alternative to this, but if you think there's one please let me know 🙏 |
||
::GRPC::GenericService.underscore(k.to_s) == grpc_method_object.name.to_s | ||
end | ||
|
||
return VALUE_UNKNOWN if method.nil? | ||
|
||
method.to_s | ||
end | ||
|
||
def extract_legacy_grpc_service(grpc_method_object) | ||
grpc_method_object.owner.to_s | ||
end | ||
|
||
def extract_legacy_grpc_method(grpc_method_object) | ||
grpc_method_object.name | ||
end | ||
|
||
def format_resource_name(grpc_method_object) | ||
grpc_method_object | ||
.owner | ||
.to_s | ||
.downcase | ||
.split('::') | ||
.<<(grpc_method_object.name) | ||
.join('.') | ||
end | ||
end | ||
|
||
# A class to extract GRPC span attributes from the full method string. | ||
class FullMethodStringFormatter | ||
# grpc_full_method is a string containing all the rpc method information (from the Protobuf definition) | ||
# in a single string with the following format: /$package.$service/$method | ||
attr_reader :grpc_full_method | ||
|
||
# resource_name is used for the span resource name. | ||
attr_reader :resource_name | ||
|
||
def initialize(grpc_full_method) | ||
@grpc_full_method = grpc_full_method | ||
@resource_name = format_resource_name(grpc_full_method) | ||
end | ||
|
||
private | ||
|
||
def format_resource_name(grpc_full_method) | ||
grpc_full_method | ||
.downcase | ||
.split('/') | ||
.reject(&:empty?) | ||
.join('.') | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
module Datadog | ||
module Tracing | ||
module Contrib | ||
module GRPC | ||
module Formatting | ||
VALUE_UNKNOWN: "unknown" | ||
class MethodObjectFormatter | ||
attr_reader grpc_full_method: String | ||
attr_reader legacy_grpc_service: String | ||
attr_reader legacy_grpc_method: String | ||
attr_reader resource_name: String | ||
|
||
def initialize: (untyped grpc_method_object) -> void | ||
|
||
private | ||
|
||
def format_full_method: (untyped grpc_method_object) -> ::String | ||
|
||
def extract_grpc_service: (untyped grpc_method_object) -> String | ||
def extract_grpc_method: (untyped grpc_method_object) -> String | ||
|
||
def extract_legacy_grpc_service: (untyped grpc_method_object) -> String | ||
|
||
def extract_legacy_grpc_method: (untyped grpc_method_object) -> String | ||
|
||
def format_resource_name: (untyped grpc_method_object) -> String | ||
end | ||
class FullMethodStringFormatter | ||
attr_reader grpc_full_method: String | ||
attr_reader resource_name: String | ||
|
||
def initialize: (String grpc_full_method) -> void | ||
|
||
private | ||
|
||
def format_resource_name: (String grpc_full_method) -> String | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
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.
Is this setting all gRPC
metadata
key/value pairs as span tags?Isn't
metadata
arbitrary user data, meaning it could have PII?If so, we can't simply tag it like this. Is this a requirement from some internal doc?
Also, the keys will become Datadog tag names, we should make sure they don't conflict our tag namespace, likely by namespacing them to
metadata.METADATA_KEY = METADATA_VALUE
.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.
Hey @marcotc 👋 !
This code was already here before my changes (I just moved the lines around)
I think the concerns that you raise make sense but I think they would be out of scope of this PR. If you wanna discuss this offline please let me know!