Skip to content

profiles: dictionary table encoding consistency improvement. #672

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

profiles: dictionary table encoding consistency improvement. #672

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
11 changes: 7 additions & 4 deletions opentelemetry/proto/profiles/v1development/profiles.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,15 @@ option go_package = "go.opentelemetry.io/proto/otlp/profiles/v1development";
// ProfilesDictionary represents the profiles data shared across the
// entire message being sent.
message ProfilesDictionary {

// Note all fields in this message MUST have a zero value encoded as the first element.
Copy link
Member

Choose a reason for hiding this comment

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

We don't have index lookups for attribute_units and a zero value encoded as the first element isn't needed for that field, so this is a little confusing as-is. Maybe we can say "all fields except attribute_units".

// This allows for _index fields pointing into the dictionary to use a 0 pointer value to indicate 'null' / 'not set'
// without requiring 'optional' encoding, which would be less compact.
// Unless otherwise defined, a 'zero value' message value is one with all default field values,
// so as to minimize wire encoded size.
Comment on lines +98 to +101
Copy link
Member

Choose a reason for hiding this comment

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

I think we can trim this text down a bit:

Suggested change
// This allows for _index fields pointing into the dictionary to use a 0 pointer value to indicate 'null' / 'not set'
// without requiring 'optional' encoding, which would be less compact.
// Unless otherwise defined, a 'zero value' message value is one with all default field values,
// so as to minimize wire encoded size.
// This allows for _index fields pointing into the dictionary to use a 0 pointer value
// to indicate 'null' / 'not set'. Unless otherwise defined, a 'zero value' message value is one with all default field
// values, so as to minimize wire encoded size.


// Mappings from address ranges to the image/binary/library mapped
// into that address range referenced by locations via Location.mapping_index.
// mapping_table[0] must always be set to a zero value default mapping,
// so that _index fields can use 0 to indicate null/unset.
repeated Mapping mapping_table = 1;

// Locations referenced by samples via Profile.location_indices.
Expand All @@ -106,8 +111,6 @@ message ProfilesDictionary {
repeated Function function_table = 3;

// Links referenced by samples via Sample.link_index.
// link_table[0] must always be set to a zero value default link,
// so that _index fields can use 0 to indicate null/unset.
repeated Link link_table = 4;

// A common table for strings referenced by various messages.
Expand Down
Loading