search

open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions
https://opentelemetry.io/docs/specs/otlp/
Apache License 2.0
606 stars 263 forks source link

profiles: drop Sample.stacktrace_id_index #575

Closed florianl closed 2 months ago

florianl commented 3 months ago

The value for Sample.stacktrace_id_index is hardly defined except for its length. It is also not defined how this value should be generated, interpreted and could be used.

Some kind of stacktrace_id can be most efficient in stateful protocols, where transmitting duplicate information should be avoided. As the OTel Profiling protocol is a stateless protocol, this element can be droped.

The proposed change is a breaking change. As the OTel Profiling signal is still marked as experimental this should be fine? Please let me know.

FYI: @open-telemetry/profiling-maintainers

jhalliday commented 3 months ago

LGTM

tigrannajaryan commented 3 months ago

Breaking change check fails since a filed is deleted: https://github.com/open-telemetry/opentelemetry-proto/actions/runs/10503389833/job/29096669193?pr=575

This is fine for Experimental status. We need to modify the breaking change detector to avoid profiling protos for now (or have a different/weaker checksfor profiling protos if that makes more sense).

I suggest to do it in a separate PR. If you propose a PR that fixes this I can merge it first, and that will unblock this PR.

felixge commented 3 months ago

I guess we should also update the model in the OTEP: open-telemetry/oteps@87b36a6/text/profiles/0239-profiles-data-model.md?plain=1#L528-L529

I'm actually not sure. I thought OTEPs are meant to capture consensus around technical decisions at a point in time, but are not intended to become evergreen documentation? 🤔

florianl commented 3 months ago

@tigrannajaryan with https://github.com/open-telemetry/opentelemetry-proto/pull/576 I have opened a PR to exclude the Profiles protocol from the breaking-changes check.

tigrannajaryan commented 3 months ago

I guess we should also update the model in the OTEP: open-telemetry/oteps@87b36a6/text/profiles/0239-profiles-data-model.md?plain=1#L528-L529

I'm actually not sure. I thought OTEPs are meant to capture consensus around technical decisions at a point in time, but are not intended to become evergreen documentation? 🤔

Correct. Any substantial changes are typically done via a new OTEP (that can amend an old OTEP). If changes are not big enough to warrant a new OTEP, they are done directly on the impacted repository.

tigrannajaryan commented 2 months ago

https://github.com/open-telemetry/opentelemetry-proto/pull/576 is merged. Please rebase, it should fix the build.

florianl commented 2 months ago

friendly ping @yurishkuro

tigrannajaryan commented 2 months ago

The PR has enough approvals, merging.

(@florianl when the PR is ready to be merged ping me I can merge it).