profiles: use single Profile.sample_type and clarify use of timestamps#649
profiles: use single Profile.sample_type and clarify use of timestamps#649tigrannajaryan merged 12 commits intoopen-telemetry:mainfrom
Conversation
Implements open-telemetry#633 (comment). Signed-off-by: Florian Lehner <florian.lehner@elastic.co> Co-authored-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
|
Should default sample type go to the scope message? |
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
That would be less flexible as it'd force us to have separate instrumentation scopes ( Do you have a use-case in mind where associating different sample types with separate instrumentation scopes would be a benefit? (I don't have a strong opinion on this, just that if we don't have a good reason to force a separate instrumentation scope per sample type, I'd go for more flexibility). |
I mostly want to make sure there is some way to capture the default sample type. The current PR removes the field IIUC. |
The main purpose of the proposal is to align values with timestamps as laid out in #633 (comment). In general google/pprof it is possible to have multiple |
@aalexand (I misread your previous comment and thought you wanted to lift |
As one example, Go heap profiles have four sample types today:
The question is how such a profile with four sample types and one of them being the preferred default as chosen by the profile producer should be encoded in OpenTelemetry profiles. Before this change it would be the array of sample types plus the default sample type. What would it be after this change and how would the default be captured? |
The current proposal doesn't have an explicit default sample type (and we probably should steer away from introducing implicit conventions) so assuming four different sample types in a single OTel messsage, samples will be grouped according to sample type to separate profiles. One possible encoding would have four different profiles, each with a different sample type (of course one could further split the data according to other criteria). Some options to accommodate the notion of a default sample type (e.g. for a consumer visualization tool) include:
1 is more restricted than 2 as it could be seen as purely advisory metadata that a visualization tool can use. Adding |
I think using nanoseconds is the better default unit for CPU profiles because the weight of a count can be different depending on the sample rate. Using the same unit for off_cpu and cpu is more consistent. The comment above was a little confusing, since it referred only to a cpu profile, but was then followed by an off_cpu profile sample as well.
profiles: improve sample_type comment
…-proto/pull/649/files#r2083519081 Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
I have added
I didn't mark |
…e_type Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
felixge
left a comment
There was a problem hiding this comment.
This LGTM, including the proposed default_sample_type field. Sorry for the delay 😞.
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
### 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)
Implements #633 (comment).
FYI: @open-telemetry/profiling-approvers