-
Notifications
You must be signed in to change notification settings - Fork 11
enhancement(core): Add efficient trace payload (aka v1) encoder #1794
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
base: main
Are you sure you want to change the base?
Changes from all commits
ed873c0
80927b4
409e801
3c64e0a
a4a1a45
c179d85
61306b3
8995438
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 |
|---|---|---|
|
|
@@ -107,6 +107,20 @@ fn main() { | |
| .customize_callback(SerdeCapableStructs) | ||
| .run_from_script(); | ||
|
|
||
| // Separate invocation for idx proto types to avoid filename collision with | ||
|
Member
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. Totally a take-it-or-leave-it thing, but it feels very opaque as an outsider to see "idx proto types" instead of just "v1"... like "v1" is immediately grokable/intuitive, but "idx" less so. Like we talk about it as the v1 protocol internally, not the "idx protocol."
Contributor
Author
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. yeah this is kinda my bad. See my other comment on trace versioning. I've started to try and talk about the project as the "Efficient trace payload" aka ETP to avoid this "V1" naming collision that happens everywhere. I went with idx in the trace-agent code since the biggest difference is that strings are 'indexed'. But I'm fine with renaming this to whatever is clearest here :P
Member
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 think ETP or v1 make the most sense since they give something more tangible / intuitive to use when discussing. |
||
| // `trace_protos/tracer_payload.rs` — both directories have a file named | ||
| // `tracer_payload.proto` so they cannot share an output directory. | ||
| protobuf_codegen::Codegen::new() | ||
| .protoc() | ||
| .includes(["proto/datadog-agent"]) | ||
| .inputs([ | ||
| "proto/datadog-agent/datadog/trace/idx/tracer_payload.proto", | ||
| "proto/datadog-agent/datadog/trace/idx/span.proto", | ||
| ]) | ||
| .cargo_out_dir("idx_trace_protos") | ||
| .customize(codegen_customize.clone()) | ||
| .run_from_script(); | ||
|
|
||
| protobuf_codegen::Codegen::new() | ||
| .protoc() | ||
| .includes(["proto/sketches-go"]) | ||
|
|
||
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.
Are these always mutually exclusive depending on payload version?
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.
structurally no, but functionally at the moment yes. The trace-agent will only ever emit one or the other in a single agent payload. I'd have to double check if trace intake can support receiving both at the same time though