Skip to content
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

Introduce specification for OTel Profiles #4197

Merged
merged 19 commits into from
Oct 24, 2024

Conversation

florianl
Copy link
Contributor

@florianl florianl commented Aug 27, 2024

Changes

Proposed change along with open-telemetry/semantic-conventions#1329 to clarify the use of build_id in the context of OTel Profiles.

FYI: @open-telemetry/profiling-maintainers

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
specification/profiles/mappings.md Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

tigrannajaryan commented Aug 27, 2024

We don't yet have anything about profiles in the spec, this would be the first. I think we need to decide what sections about profiling we want in the spec. Is it going to be data model description?

Is data model going to be decoupled from its OTLP representation like we do for other signals (see log data model vs log proto or metric data model vs metric proto)?

Or do we think that OTLP representation is all we care about? In that case this clarification about Mappings could be added to proto.

@open-telemetry/profiling-maintainers @open-telemetry/spec-sponsors thoughts?

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@felixge
Copy link
Member

felixge commented Aug 28, 2024

Is data model going to be decoupled from its OTLP representation like we do for other signals [...]

Or do we think that OTLP representation is all we care about? In that case this clarification about Mappings could be added to proto.

My initial preference would be to not maintain a separate data model description and focus on the OTLP representation. I'm assuming we can always come back to it and add an abstract data model description later?

@jsuereth
Copy link
Contributor

My initial preference would be to not maintain a separate data model description and focus on the OTLP representation. I'm assuming we can always come back to it and add an abstract data model description later?

In my mind they're effectively the same thing, but the data model is a bit richer / filled with descriptions on usage and advanced concepts that may not be in the protocol.

E.g. a description on how to associate symbols back to profiles outside of the raw OTLP send is in your data model.

I agree that focusing on OTLP representaiton is priority #1 - but that's effectively working on the data model. You need a location to put information about how to consume and interpret the OTLP that goes beyond just "here's what this one message means".

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there is still active discussion about how to organize this new content, but if you do decide to create new section(s) then, see the inline suggestion.

specification/profiles/mappings.md Show resolved Hide resolved
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Copy link

github-actions bot commented Sep 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 6, 2024
florianl and others added 2 commits September 10, 2024 11:01
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl
Copy link
Contributor Author

Added the hash specification for process.executable.build_id.profiling` for open-telemetry/semantic-conventions#1329.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
specification/profiles/mappings.md Outdated Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
specification/profiles/mappings.md Outdated Show resolved Hide resolved
specification/profiles/mappings.md Outdated Show resolved Hide resolved
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
@github-actions github-actions bot removed the Stale label Sep 11, 2024
@trask
Copy link
Member

trask commented Sep 26, 2024

is there something I can do to continue with this specification?

hey @florianl! stuck spec PRs are not uncommon 😅 and there is some general guidance here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#how-to-get-your-pr-merged

in this case I'd recommend reaching out to the profiling folks in the Profiling SIG meeting and in the #otel-profiling slack channel if you haven't already

Copy link

github-actions bot commented Oct 4, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 4, 2024
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

specification/profiles/mappings.md Show resolved Hide resolved
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks 🙇

florianl and others added 2 commits October 14, 2024 20:49
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 24, 2024
@florianl
Copy link
Contributor Author

friendly ping @open-telemetry/technical-committee - can you provide some feedback why this change shouldn't move forward?

@christos68k
Copy link
Member

friendly ping @open-telemetry/technical-committee - can you provide some feedback why this change shouldn't move forward?

AFAIK no such decision was made. Github will mark PRs stale based on activity and close them after a few days. Usually someone with permissions to do so manually removes the label before that happens but we can always reopen the closed PR.

@tigrannajaryan tigrannajaryan added spec:profiling Related to the specification/profiling directory and removed Stale labels Oct 24, 2024
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, approving to make progress.

My question regarding what exactly we want to have in the spec repo for profiles (data model vs protocol details) I think still remains, but it is non blocking.

@open-telemetry/profiling-maintainers Please think about what structure of documents you would like to have for profiling in this repo.

@tigrannajaryan
Copy link
Member

This has enough approvals, including 2 from @open-telemetry/profiling-maintainers, so I am merging.

@tigrannajaryan tigrannajaryan merged commit 8548dfc into open-telemetry:main Oct 24, 2024
6 checks passed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
## Changes

Proposed change along with open-telemetry/semantic-conventions#1329 to clarify the use of `build_id` in the context of OTel Profiles.

FYI: @open-telemetry/profiling-maintainers 

* [x] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:profiling Related to the specification/profiling directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants