processcontext: add ProcessContext message from OTEP 4719#783
processcontext: add ProcessContext message from OTEP 4719#783ivoanjo wants to merge 5 commits intoopen-telemetry:mainfrom
ProcessContext message from OTEP 4719#783Conversation
As open-telemetry/opentelemetry-specification#4719 looks to be merged soon, it came up as we were implementing this spec in the OTel eBPF Profiler (open-telemetry/opentelemetry-ebpf-profiler#1181) that it'd be nice to stop copy-pasting the `process_context.proto` and it's time to add it to the proper place. This is my first contribution to this repo so please do point out if I missed something! I didn't touch the collector parts since this message is not expected to be processed by the collector directly.
ProcessContext message from OTEP 4719ProcessContext message from OTEP 4719
Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>
| // duplicated keys can be unpredictable. | ||
| // | ||
| // Attributes SHOULD follow OpenTelemetry semantic conventions where applicable. | ||
| // See: https://opentelemetry.io/docs/specs/semconv/ |
There was a problem hiding this comment.
NIT: Is this verbose description of the attribute semantics meant to indicate that the requirements here are different from the ResourceProfiles message?
There was a problem hiding this comment.
requirements here are different
No, afaik they should be the same as we carry in resource profiles. Do you think it would be clearer to add something along the lines of "A reader such as the OTel eBPF profiler is expected to carry this info in the ResourceProfiles message"?
There was a problem hiding this comment.
I was mostly thinking that the comments for this field should be the same across all messages that use it in the same way.
There was a problem hiding this comment.
Ack! In ec83a05 I've tweaked the comment to match profiles.proto.
In hindsight this was a bit of "I'm looking but I'm not actually seeing it anymore" -- the previous comment had evolved from a time where we had attributes, not a resource directly, and was lifted from resource.proto, but doesn't make a lot of sense now that we embed the whole resource as a nested message.
tigrannajaryan
left a comment
There was a problem hiding this comment.
I would like to discuss if this is the right repo for this proto.
Currently the repo description is "OpenTelemetry protocol (OTLP) specification and Protobuf definitions" and I do not think what is being added is part of OTLP.
We have a few options:
- Use a separate repo
- Explicitly extend the purpose of the repo
- Clarify that this is indeed part of OTLP (how?)
|
We discussed in the specification meeting. A few thoughts:
@florianl / @ivoanjo - should we open an issue to track this decision and mark it as a blocker to stabilizing the process-context protocol? |
|
+1 I think it makes a lot of sense to make extra sure we're happy with this decision (or change it) prior to stabilizing it, and for now experiment with having it here during development and check how useful/not useful that turns out. |
Some additional context:
|
That's only the repo name. The content of the repo currently is strictly OTLP. The README talks about OTLP, the "docs" directory is all about OTLP. I do not mind hosting other *.proto files in this repo, as the name of the repo implies. But if we want to do that at least we must update the README and somehow reflect that "docs" are only about OTLP. |
|
In my (very humble) opinion, the really nice plus of having it in this repo is making it really easy to consume/distribute this protobuf message definition. Having it in a whole different location would mean something along the lines of:
It sounds like there's some openness to keep it in this repo if we update the docs to avoid the confusion of "is this just OTLP or what?; I might be slow over the next few days (flying to Japan to speak at RubyKaigi conference!) but I'll push an update to this PR to update docs ASAP so y'all can see if that direction looks like it's good or not :) |
|
Following the repo's description "OpenTelemetry protocol (OTLP) specification and Protobuf definitions" it reads to me as it should not be limited to OTLP only. But I also agree, that some parts of the README should be rephrased to clarify this point. On the practical side, having this here means the OTel ecosystem can benefit from it without hunting for it in another place. Plus, spinning up a whole separate repo seems like overkill, as it would duplicating CI, releases and maintenance overhead. Since these proposed changes are marked as development, we're not committing to anything forever. If down the road it makes sense to move it or remove it, we still can do that. |
As open-telemetry/opentelemetry-specification#4719 looks to be merged soon, it came up as we were implementing this spec in the OTel eBPF Profiler
(open-telemetry/opentelemetry-ebpf-profiler#1181) that it'd be nice to stop copy-pasting the
process_context.protoand it's time to add it to the proper place.This is my first contribution to this repo so please do point out if I missed something! I didn't touch the collector parts since this message is not expected to be processed by the collector directly.