-
Notifications
You must be signed in to change notification settings - Fork 6
Add development protobuf definition for process context #13
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 development protobuf definition for process context #13
Conversation
This PR adds the protobuf definition for the specification proposed in <https://docs.google.com/document/d/1-4jo29vWBZZ0nKKAOG13uAQjRcARwmRc4P313LTbPOE/edit?tab=t.0>. The `OtelProcessCtx` message is used to allow in-process libraries (such as the OTel SDKs running inside an application) to share information with outside readers (such as the OpenTelemetry eBPF Profiler). At this point, it's not clear to me if this is the final home for this file/the specification, but it looks like a good home for now while we develop and iterate on it.
| // Similar to baggage https://opentelemetry.io/docs/concepts/signals/baggage/ / https://opentelemetry.io/docs/specs/otel/overview/#baggage-signal | ||
| map<string, string> resources = 1; | ||
| // https://opentelemetry.io/docs/specs/semconv/registry/attributes/deployment/#deployment-environment-name | ||
| string deployment_environment_name = 2; |
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.
My concern here is that there is no documentation or guideline about where resource attributes should end up (map<> vs dedicated field). What promotes dedicated attributes to have a dedicated field in the proto?
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.
My current thinking/proposal for making this distinction is:
-
Dedicated field => Standardized attributes that most apps are expected to fill, with semantics matching existing semantic conventions
-
Resources => Free-form payload that can be used to pass along data. E.g. from https://opentelemetry.io/docs/concepts/signals/baggage/ :
Baggage is contextual information that resides next to context. Baggage is a key-value store, which means it lets you propagate any data you like alongside context.
Baggage means you can pass data across services and processes, making it available to add to traces, metrics, or logs in those services.
and from https://opentelemetry.io/docs/specs/otel/overview/#baggage-signal :
While Baggage can be used to prototype other cross-cutting concerns, this mechanism is primarily intended to convey values for the OpenTelemetry observability systems.
These values can be consumed from Baggage and used as additional attributes for metrics, or additional context for logs and traces.
I only recently learned of the concept of baggage but I believe matches the semantics we should go for here. Perhaps we should rename resources => baggage to be more explicit even?
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.
Added a comment about this in d239ca7
| message OtelProcessCtx { | ||
| // Additional key/value pairs as resources https://opentelemetry.io/docs/specs/otel/resource/sdk/ | ||
| // Similar to baggage https://opentelemetry.io/docs/concepts/signals/baggage/ / https://opentelemetry.io/docs/specs/otel/overview/#baggage-signal | ||
| map<string, string> resources = 1; |
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.
What happens if this field conflicts with the hard-coded conventions you have?
Also what happens if any of the conventions in the below proto are empty?
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.
What happens if this field conflicts with the hard-coded conventions you have?
Good question! We propose that hard-coded conventions always take priority. Readers can choose to fallback to a value in a resources field if the hard-coded field is empty, or can ignore it.
Also what happens if any of the conventions in the below proto are empty?
Our proposal on this (from the spec doc) is
"The reference implementation strongly recommends that callers provide the following keys (from the existing semantic conventions), but they can be empty if needed"
In particular deployment_environment_name and service_version I believe are the ones that will commonly be blank. service_name has well-defined fallback values, telemetry_sdk_* can be trivially hardcoded in every implementation and service_instance_id can be generated.
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.
Added a comment about this in e50806d
|
The upstream OTEP open-telemetry/opentelemetry-specification#4719 has been changed to no longer use For that reason, this PR is no longer required, so closing! Thanks for the feedback so far -- this was very useful in getting the OTEP into shape. |
This PR adds the protobuf definition for the specification proposed in https://docs.google.com/document/d/1-4jo29vWBZZ0nKKAOG13uAQjRcARwmRc4P313LTbPOE/edit?tab=t.0.
The
OtelProcessCtxmessage is used to allow in-process libraries (such as the OTel SDKs running inside an application) to share information with outside readers (such as the OpenTelemetry eBPF Profiler).At this point, it's not clear to me if this is the final home for this file/the specification, but it looks like a good home for now while we develop and iterate on it.