Conversation
|
I recommend to disable whitespaces to review this PR, most of the diff is just indentation due |
andrewhr
changed the title
andrewhr
changed the title
| def request_rest( | ||
| %Client{} = client, | ||
| %ServiceMetadata{} = metadata, | ||
| action, |
There was a problem hiding this comment.
This action does not exist for REST services (at least not available now). The closest thing to it sees to be conjunction of HTTP method and the path. WDYT of using that for the action field?
We could do something like:
"#{http_method |> Atom.to_string() |> String.upcase()} #{path}"There was a problem hiding this comment.
I've added those on new generated code.
The reason of why action is desirable, by OpenTelemetry docs (and some reference implementation) the spans should be named as ServiceName.OperationName.
Does that makes sense?
|
Hey @philss, thanks for taking time to review! Sorry, I didn't made the message that clear. With little changes to code generator, we can have those I've kept the changes to auto-generated code in a separate commit to help you review how that looks like. Thanks once more for your time! 🙇 |
|
@andrewhr oh man, sorry. I didn't read the description entirely. That makes sense! 👍 |
This patch add Telemetry support for all AWS requests. For context, what this patch aims is to eventually map those Telemetry spans into [semantic OpenTelemetry traces][1]. This inform what kind of metadata we need to provide on those spans: * Client: which itself includes useful information like target region. * ServiceMetadata: this is the most important one, with info about the target Service itself; * Action: the operation name, more on that bellow; * Input: the input sent by the user, more on that bellow; The `action` value is not provided on REST requests, and requires to re-generate code. A companion PR can be sent to aws-codegen project, it this feature is accepted. Given `AWS.Request` is a private implementation for the generated code, I assume this change is not breaking from user perspective. The `input` AFAICT would be useful to extract service-specific information like, for example, the table names of DynamoDB. [1]: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/aws-sdk.md
andrewhr
force-pushed
the
telemetry-spans-for-operations
branch
from
2d1e4a2 to
2aaeda5
Compare
|
Sorry @philss , this PR was accumulating dust in my hard drive. I rebased against latest master - which includes the ServiceMedata changes - and published the companion PR here aws-beam/aws-codegen#90. Thanks a bunch! 🙇 |
|
@andrewhr no problem! I will check both PRs later today. Thank you! 💜 |
|
Is this still in the works? |
I pinged the owner of the PR some months ago but didn't get a reply: aws-beam/aws-codegen#90 (comment) I'll try and sort this out in the weekend 👍 |
4 participants
This patch add Telemetry support for all AWS requests.
For context, what this patch aims is to eventually map those Telemetry spans into semantic OpenTelemetry traces. This inform what kind of metadata we need to provide on those spans:
The
actionvalue is not provided on REST requests, and requires to re-generate code. A companion PR can be sent to aws-codegen, it this feature is accepted. GivenAWS.Requestis a private implementation for the generated code, I assume this change is not breaking from user perspective.The
inputAFAICT would be useful to extract service-specific information like, for example, the table names of DynamoDB.