Update profiles specification README.md and pprof.md#4932
Update profiles specification README.md and pprof.md#4932christos68k wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
This update take into account the recent introduction of a profiles signal concepts page, removes duplicate information and adds more relevant documentation.
|
|
||
| ## Overview | ||
|
|
||
| Profiles are emerging as the fourth essential signal of observability, alongside |
There was a problem hiding this comment.
I previously moved this overview to the Profiles concepts page.
specification/profiles/pprof.md
Outdated
| and back again into [pprof](https://github.com/google/pprof/tree/main/proto) without | ||
| data loss (convertibility but not wire compatibility). | ||
|
|
||
| To enable this compatibility through explicit conversion, OpenTelemetry provides a `pprof` [namespace](https://opentelemetry.io/docs/specs/semconv/general/profiles/#compatibility-with-pprof) |
There was a problem hiding this comment.
Update the link to the pprof namespace and not the copy of some of this namespace in the Profiles space.
| To enable this compatibility through explicit conversion, OpenTelemetry provides a `pprof` [namespace](https://opentelemetry.io/docs/specs/semconv/general/profiles/#compatibility-with-pprof) | |
| To enable this compatibility through explicit conversion, OpenTelemetry provides a `pprof` [namespace](https://opentelemetry.io/docs/specs/semconv/registry/attributes/pprof/) |
There was a problem hiding this comment.
I'll update the wording here to remove the reference to 'namespace' and keep the existing link as I think it's more helpful (it contains additional explanatory text that gives more context and helps frame the attributes that follow).
EDIT: See 6b4382e
|
|
||
| > [!NOTE] | ||
| > | ||
| > The profiles signal is still experimental and under active development. |
There was a problem hiding this comment.
We should rephrase this note to sound less concerning.
| > The profiles signal is still experimental and under active development. | |
| > The profiles signal is currently marked as Alpha under development. |
There was a problem hiding this comment.
This is the same preamble as the one I added in Profiles concepts (which already went live). Since the signal isn't yet Alpha, I think this is more accurate.
We can update this to Alpha shortly before or immediately after the announcement.
There was a problem hiding this comment.
Agreed. It's not yet marked as alpha (see this PR).
But maybe we don't need this whole paragraph since the Status is already declared above. Other documents with status "Development" don't seem to have such a Notes section either.
Anyway, since it's already in the other doc, I'm also fine with keeping it for now.
There was a problem hiding this comment.
We should probably gather ourselves a list of all places that need to be coherently updated on the Alpha day.
There was a problem hiding this comment.
| - **Generalized attributes**: Most messages can carry attributes following the | ||
| same conventions as other signals, augmented with Unit information (`KeyValueAndUnit`). |
There was a problem hiding this comment.
As the topic around KeyValueAndUnit is under discussion and there will be some change, we should not point it out here. To me, its an implementation detail rather than a key concept in the data model.
Also, I think it does not add more to the data model that is not yet covered by **Generalized dictionary**.
| - **Generalized attributes**: Most messages can carry attributes following the | |
| same conventions as other signals, augmented with Unit information (`KeyValueAndUnit`). |
There was a problem hiding this comment.
I think we absolutely need to mention this here for two main reasons:
- This is a major differentiator from pprof
- It doesn't follow existing OTel conventions so questions may arise
I think one job of the spec is to clarify such corner cases instead of hiding them. Moreover, this is not just an implementation detail but a core aspect of the data model, which I'll also try to explain in data-model.md.
|
|
||
| > [!NOTE] | ||
| > | ||
| > The profiles signal is still experimental and under active development. |
There was a problem hiding this comment.
Agreed. It's not yet marked as alpha (see this PR).
But maybe we don't need this whole paragraph since the Status is already declared above. Other documents with status "Development" don't seem to have such a Notes section either.
Anyway, since it's already in the other doc, I'm also fine with keeping it for now.
Co-authored-by: Felix Geisendörfer <felix@felixge.de>
Co-authored-by: Felix Geisendörfer <felix@felixge.de>
Co-authored-by: Felix Geisendörfer <felix@felixge.de>
| compatible with OpenTelemetry Profiles in a sense that it can be transformed into | ||
| OpenTelemetry Profiles and again into [pprof](https://github.com/google/pprof/tree/main/proto) | ||
| without data loss. | ||
| Original [pprof](https://github.com/google/pprof/tree/main/proto) is compatible |
There was a problem hiding this comment.
Should we merge this pprof.md file into the README.md, as a section? It's not a lot of text, so it might be easier to just have the single README.md file.
There was a problem hiding this comment.
Given that we want really good integration with pprof due to its importance, I was thinking we'd want to expand the pprof information at some point, once we have a better/clearer story to tell there regarding conversions, recommendations, maybe an example and so on.
In that way it's similar to mappings.md, but I have no strong opinion on this for now (maybe if we leave it as a separate page it'll act as a nudge for us to improve it). If you feel strongly that we should merge it into the README, LMK and I will update.
There was a problem hiding this comment.
Good point about expanding this at some point and the separate file being a nudge. SG to keep it separate.
This update take into account the recent introduction of a profiles signal concepts page, removes duplicate information and adds more relevant documentation.
There will be a follow-up PR to add the current data model page. Do NOT merge this PR until the follow-up is also reviewed and approved as it currently links to a page (
./data-model.md) that does not exist.CC: @theletterf @open-telemetry/profiling-maintainers @open-telemetry/profiling-approvers
Changes
Please provide a brief description of the changes here.
For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check