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.

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

profile: drop duplicate field in message Profile #606

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

florianl
Copy link
Contributor

@florianl florianl commented Dec 6, 2024

The message Profile contains two fields of type opentelemetry.proto.common.v1.KeyValue, attribute_table and attributes. Both fields are intended for the same use. Therefore drop the later one.

ping @open-telemetry/profiling-maintainers @jhalliday

The message Profile contains two fields of type `opentelemetry.proto.common.v1.KeyValue`, `attribute_table` and `attributes`. Both fields are intended for the same use. Therefore drop the later one.
@florianl florianl requested review from a team as code owners December 6, 2024 12:54
@jhalliday
Copy link
Contributor

yes, not actually duplicates. The attributes_table is there to be referenced by fields of attributes in other places as a de-duplication / size reduction measure, whereas the attributes field is for things that belong directly to the profile itself. It would be correct, though not especially useful, to make the attributes field a repeated int indexing into the attributes table, but removing it entirely is not appropriate.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl
Copy link
Contributor Author

florianl commented Dec 6, 2024

Thanks for the feedback. The differences between attribute_table and attributes were not clear, so that gave me the impression of a duplicate field. To address your feedback, I have added attribute_indices with bcfb44e for the global attributes in message Profile, that can reference into attribute_table.
This should help differentiating the attributes while having only a single attribute_table which also helps to deduplicate elements.

// "abc.com/myattribute": true
// "abc.com/score": 10.239
//
repeated int32 attribute_indices = 22;
Copy link

Choose a reason for hiding this comment

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

Do we want to simply reuse the field ID of the removed field maybe? It's not a backward compatible change, but we recently did a field renumbering and I assume before releasing version 1 we'd want to make sure we don't have ID gaps, so we might as well do this now. @jhalliday WDYT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the moment I would vote for just using a new field ID and not reuse the existing one, change its field name and field type. Before declaring this proto as stable, closing these gaps make sense, but I think this will still take a while.

Copy link

Choose a reason for hiding this comment

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

SGTM

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@jhalliday
Copy link
Contributor

@dmathieu does this have the potential to cause headaches in the collector? Adding attributes to the 'top level' entity in a given signal type seems like one of the most common mutations in the collector pipeline use cases and I think all the others use common.v1.KeyValue and hence probably have shared pdata golang types/code for doing so. Are we adding undue complexity here if profiles works a different way?

@dmathieu
Copy link
Member

Processors should be able to get a command to "add an attribute to a profile/sample", and have that turned into the proper append/find in the attributes table, and indices.
So I don't think it's so much of an added complexity.

It's true that the entirety of these mapping tables are pretty unique to profiles, and none of the other signals use it though, making profiles rather a snowflake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants