-
Notifications
You must be signed in to change notification settings - Fork 297
profiles: remove default_sample_type #679
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
Conversation
7a98df8
to
23bdc47
Compare
0601985
to
cb85d1c
Compare
@felixge might be good to have a changelog entry? |
The profiling signal is still experimental and WIP, we don't add changelog entries for its changes right now. We will start doing this once 1.0 version is out. |
In the last SIG meeting I promised that I would provide an example of how a Example pprof input (inspired by the Go memory profiles, but simplified):
Converted to OpenTelemetry:
This should round trip back to the original pprof file. However, there is a small ambiguity here when Anyway, I don't think this is a blocker for this PR. It's certainly a solvable problem. |
This resolves the ambiguity when a `ScopesProfiles` message contains more than one `profiles` entry matching the `default_sample_type`. Also add a comment to clarify which profile viewers should display by default. This was previously also ambiguous. This change creates a minor problem when it comes to ensuring that pprof profiles can round-trip through otel conversion as it may require the `ScopeProfiles.profiles` entries to be reordered in order to honor the `default_sample_type` of the original pprof message. Naively converting the resulting otel profile back to pprof would cause the order of `Profile.sample_types` to be changed. Merging this change indiciates consensus within the profiling group that this issue should be handled by adding a `pprof.profile_order` attribute (name TBD) to the `ScopeProfiles.attributes` during the initial pprof->otel conversion. This label will allow converting the resulting otel profile back to pprof without any information loss. See [open-telemetrygh-633][] for additional details. [open-telemetrygh-633]: open-telemetry#633 (comment)
cb85d1c
to
a13103f
Compare
This should be ready for merge now 🙇 . My force-push was a rebase. |
### Changed - profiles: drop gzip requirement. [#661](#661) - profiles: avoid `optional` keyword usage. [#659](#659) - profiles: make `profile_id` optional. [#665](#665) - profiles: use single `Profile.sample_type` and clarify use of timestamps. [#649](#649) - all: add notes about the attribute values restrictions. [#683](https://github.com/open-telemetry/opentelemetry-proto/pull/683)<br>⚠️ **IMPORTANT**: These restrictions can be dropped in a future minor release. - profiles: clarify usage of the zero value as the first element of tables in `ProfilesDictionary`. [#688](#688), [#698](#698) - profiles: unsigned `time_nanos` and `duration_nanos` in `Profile`. [#692](#692) - profiles: improve attribute encoding in `ProfilesDictionary`. [#672](#672) - profiles: simplify profile stack trace representation. [#708](#708) ### Fixed - examples: fix OTLP JSON Event example body. [#666](#666) - docs: minor specification fixes around `UNAVAILABLE` and `RetryInfo`. [#669](#669) ### Removed - profiles: remove `default_sample_type`. [#679](#679) - profiles: remove `has_*` debug info fields, they are moving to attributes. [#595](#595) - profiles: remove `Location.is_folded`. [#690](#690)
This resolves the ambiguity when a
ScopesProfiles
message containsmore than one
profiles
entry matching thedefault_sample_type
.Also add a comment to clarify which profile viewers should display by
default. This was previously also ambiguous.
This change creates a minor problem when it comes to ensuring that pprof
profiles can round-trip through otel conversion as it may require the
ScopeProfiles.profiles
entries to be reordered in order to honor thedefault_sample_type
of the original pprof message. Naively convertingthe resulting otel profile back to pprof would cause the order of
Profile.sample_types
to be changed.Merging this change indiciates consensus within the profiling group that
this issue should be handled by adding a
pprof.profile_order
attribute(name TBD) to the
ScopeProfiles.attributes
during the initialpprof->otel conversion. This label will allow converting the resulting
otel profile back to pprof without any information loss.
See gh-633 for additional details.