Skip to content

profiles: Make non-zero duration being delta relationship explicit #848

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.

Sign up for GitHub

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

Open
brancz wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −5
Open

profiles: Make non-zero duration being delta relationship explicit #848

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions opentelemetry/proto/profiles/v1development/profiles.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,13 @@ message Profile {

// Time of collection (UTC) as nanoseconds since the UNIX epoch.
fixed64 time_unix_nano = 3;
// Duration of the profile in nanoseconds. For instant profiles like
// live heap snapshot, the duration can be zero but it may be preferable
// to set time_unix_nano to the process start time and duration_nano to
// the relative time when the profile was gathered so that Sample.timestamps_unix_nano
// values fall within the profile time range.
// Duration of the profile in nanoseconds. For instant profiles like live
// heap snapshot, the duration can be zero but it may be preferable to set
// time_unix_nano to the process start time and duration_nano to the relative
// time when the profile was gathered so that Sample.timestamps_unix_nano
// values fall within the profile time range. If duration is larger than
// zero, then the samples describe a delta of occurrences in the
// [ time_unix_nano, time_unix_nano + duration_nano ) window.
Comment on lines +304 to +310

@florianl florianl Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about rephrasing it to something like this?

Suggested change
// Duration of the profile in nanoseconds. For instant profiles like live
// heap snapshot, the duration can be zero but it may be preferable to set
// time_unix_nano to the process start time and duration_nano to the relative
// time when the profile was gathered so that Sample.timestamps_unix_nano
// values fall within the profile time range. If duration is larger than
// zero, then the samples describe a delta of occurrences in the
// [ time_unix_nano, time_unix_nano + duration_nano ) window.
// Duration of the profile in nanoseconds.
// If zero, the profile is an instant snapshot, like a live heap snapshot.
// If non-zero, the profile has delta semantics, where sample values
// reflect occurrences within [time_unix_nano, time_unix_nano + duration_nano),
// not cumulative totals.

To me, the part it may be preferable to set time_unix_nano to the process start time and duration_nano to the relative time when the profile was gathered so that Sample.timestamps_unix_nano values fall within the profile time range. sounds to vage. What does preferable mean and how can users differentiate this preferable interpretation to other interpretations?

@aalexand aalexand Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current comment is from this discussion in the past.

The idea was to allow Sample timestamps (Sample.timestamps_unix_nano) in instantaneous profiles (like live heap snapshot) and also always maintain the Sample.timestamps_unix_nano ∈ [Profile.time_unix_nano, Profile.time_unix_nano+Profile.duration_nano) invariant. If we say that instantaneous profiles must have zero duration then technically it means that for such profiles the sample timestamps are either disallowed or they point outside the profile time range and we wanted to avoid this special casing.

@aalexand aalexand Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is also how we check the timestamps in profcheck today - here.

@florianl florianl Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm open to further refinement, I suggest we follow our previous discussion and use clear, relevant prefixes, such as Profile.time_unix_nano, Profile.duration_nano, and Sample.timestamps_unix_nano to prevent confusion. Additionally, to provide explicit guidance for implementers, we should replace vague phrasing like [...] it may be preferable to set [...] with more definitive language.

uint64 duration_nano = 4;
// The type and the unit of the events between sampled occurrences for
// periodic sampling profiles. It can be the same as sample_type or it can be
Expand Down
Loading