Skip to content
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

profiles: move attribute_table to message ScopeProfiles #609

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

profiles: move attribute_table to message ScopeProfiles #609

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a0dee7
profiles: move attribute_table to message ScopeProfiles
florianl Dec 9, 2024
55bbd73
fixup: move string_table from message Profile to message ScopeProfiles
florianl Dec 9, 2024
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: 6 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 @@ -144,6 +144,9 @@ message ScopeProfiles {
// https://opentelemetry.io/docs/specs/otel/schemas/#schema-url
// This schema_url applies to all profiles in the "profiles" field.
string schema_url = 3;

// Lookup table for attributes.
repeated opentelemetry.proto.common.v1.KeyValue attribute_table = 4;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The argument can be made to even move attribute_table to ResourceProfiles or ProfilesData instead of ScopeProfiles. If you prefer this, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

ProfilesData doesn't seem like a good fit as if my understanding is correct it can be populated by intermediate processors that receive data from multiple sources.

ResourceProfiles can also contain data from multiple instrumentation scopes so we'd be introducing implicit merging of key values requirement if we moved it there.

Doesn't the same reasoning (reduce amount of duplicate data) also apply to moving the string table?

Copy link
Member

Choose a reason for hiding this comment

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

Also, should we merge #606 into this PR, otherwise it can be a little confusing (e.g. doesn't update Profile attributes to be references)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not moving attribute_table for the named reason to ProfilesData or ResourceProfiles sounds good to me. When moving "up" attributes_table to ScopeProfiles I just looked at it from a protocol perspective and not from a potential synchronization issues of processors.

Doesn't the same reasoning (reduce amount of duplicate data) also apply to moving the string table?

yes - the same applies to string_table. I can add it here as well.

Also, should we merge #606 into this PR, otherwise it can be a little confusing (e.g. doesn't update Profile attributes to be references)

To me, these are independent proposals that should be handled and decided on separately. So I didn't merge everything into a big PR. Depending on which PR gets merged first, the other still can be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved string_table with 55bbd73 as well to ScopeProfiles.

}

// Profile is a common stacktrace profile format.
Expand Down Expand Up @@ -201,8 +204,6 @@ message Profile {
repeated int32 location_indices = 5;
// Functions referenced by locations.
repeated Function function_table = 6;
// Lookup table for attributes.
repeated opentelemetry.proto.common.v1.KeyValue attribute_table = 7;
// Represents a mapping between Attribute Keys and Units.
repeated AttributeUnit attribute_units = 8;
// Lookup table for links.
Expand Down Expand Up @@ -383,7 +384,7 @@ message Sample {
// result has a list of values that is the element-wise sum of the
// lists of the originals.
repeated int64 value = 3;
// References to attributes in Profile.attribute_table. [optional]
// References to attributes in ScopeProfiles.attribute_table. [optional]
repeated int32 attribute_indices = 4;

// Reference to link in Profile.link_table. [optional]
Expand All @@ -407,7 +408,7 @@ message Mapping {
// disk for the main binary and shared libraries, or virtual
// abstractions like "[vdso]".
int32 filename_strindex = 4; // Index into string table
// References to attributes in Profile.attribute_table. [optional]
// References to attributes in ScopeProfiles.attribute_table. [optional]
repeated int32 attribute_indices = 5;
// The following fields indicate the resolution of symbolic info.
bool has_functions = 6;
Expand Down Expand Up @@ -443,7 +444,7 @@ message Location {
// profile changes.
bool is_folded = 4;

// References to attributes in Profile.attribute_table. [optional]
// References to attributes in ScopeProfiles.attribute_table. [optional]
repeated int32 attribute_indices = 5;
}

Expand Down
Loading