Add new profile signal - Githubissues

open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions

https://opentelemetry.io/docs/specs/otlp/

Apache License 2.0

545 stars 242 forks source link

Add new profile signal #534

Closed petethepig closed 2 months ago

petethepig commented 3 months ago

This is a follow up to OTEP 239: Introduces Profiling Data Model v2

The main motivation behind this PR is that this will allow us to start experimenting with the profiles proto in opentelemetry-collector. I marked the profiles part as Experimental to indicate that this is not a final version of the data model.

I copied the proto from the OTEP, and moved pprofextended.proto from profiles/v1/alternatives/pprofextended.proto to just profiles/v1/pprofextended.proto. I did this because I figured we no longer have alternative representations and this will reduce confusion for people outside of Profiling SIG. The rest of the proto stayed the same.

I tested this file with a collector fork and I it compiles properly.

tigrannajaryan commented 3 months ago

The recommendation is to put experimental signals in "experimental" subdirectory.

@open-telemetry/specs-approvers any thoughts on whether want to use something different to indicate the state of profiles in this repo?

tigrannajaryan commented 3 months ago

This formally has enough approvals, but I would like to keep it open in case any other @open-telemetry/specs-approvers want to review.

tigrannajaryan commented 2 months ago

@petethepig can you please confirm that the format defined here is backward-compatible with current pprof format in the sense that:

This format only adds new fields. No fields are deleted and there are no changes to existing fields.
Any valid profile represented in the current pprof format is considered a valid Otel profile data.

Is this correct?

petethepig commented 2 months ago

@tigrannajaryan yes, these two things are correct.

tigrannajaryan commented 2 months ago

@petethepig Please address all open comments and resolve them and then we can merge it.

@open-telemetry/specs-approvers unless I hear objections I am going to merge this.

petethepig commented 2 months ago

@tigrannajaryan I addressed the comments and resolved the outstanding conversations.

tigrannajaryan commented 2 months ago

@jack-berg you had several comments, you OK with merging in the current state?

tigrannajaryan commented 2 months ago

We have a rule of keeping spec and proto PRs open for 2 days since last change. I will merge this tomorrow.

petethepig commented 2 months ago

@tigrannajaryan could you please merge it if everything looks good, as it's been a few days since the last update? Thank you!