Skip to content

Move the profiles lookup tables into a new Dictionary message#650

Merged
reyang merged 8 commits intoopen-telemetry:mainfrom
dmathieu:profiles-reference-table
May 15, 2025
Merged

Move the profiles lookup tables into a new Dictionary message#650
reyang merged 8 commits intoopen-telemetry:mainfrom
dmathieu:profiles-reference-table

Conversation

@dmathieu
Copy link
Copy Markdown
Member

@dmathieu dmathieu commented May 5, 2025

As discussed in #648

@dmathieu dmathieu force-pushed the profiles-reference-table branch 2 times, most recently from a7fc829 to a06fd08 Compare May 5, 2025 08:01
@dmathieu dmathieu force-pushed the profiles-reference-table branch from a06fd08 to 7935a29 Compare May 5, 2025 08:03
@dmathieu dmathieu marked this pull request as ready for review May 5, 2025 08:04
@dmathieu dmathieu requested review from a team May 5, 2025 08:04
Comment thread opentelemetry/proto/profiles/v1development/profiles.proto Outdated
Comment thread opentelemetry/proto/collector/profiles/v1development/profiles_service.proto Outdated
Copy link
Copy Markdown
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

We should also update the references in documentation strings from ProfilesData.XY_table to refer to ProfilesDictionary.XY_table.

Comment thread opentelemetry/proto/profiles/v1development/profiles.proto Outdated
dmathieu and others added 2 commits May 6, 2025 09:12
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
@jhalliday
Copy link
Copy Markdown
Contributor

Thanks, Damien. I like it, this feels like a more refined direction than #648 was.

I agree with points on streamlining terminology. Mixing 'reference' and 'lookup' isn't great. 'lookup_tables' field should probably become 'reference_tables' for consistency with the message name? Likewise if going with 'Dictionary'. I think the use of _index/_indices for pointers into the tables is already consistent, though if we're going with 'reference' everywhere I'm inclined to _ref/_reference/_references suffixes as an alternative and that may be a point in favour of using 'Reference' over 'Dictionary' for the message type. Whilst I'm the reason _strindex exists as a special distinction, I'm no longer sold on its advantages, it could be simplified to _index/_ref without great loss.

string_table is the only field likely to be common across ${signal}ReferenceTables messages in future, there may be some small clarity/consistency advantage to establishing a convention of ordering it as the first field in the message.

The idea of adding a 'bytes tables_id' field in ProfilesReferenceTables to allow for separately transmitted messages referencing into a particular set of tables rather than one default/bundled one still has some attraction, but at present I'm leaning towards leaving it out as YAGNI.

Copy link
Copy Markdown
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM. I like this direction! Just got some nits.

Comment thread opentelemetry/proto/profiles/v1development/profiles.proto Outdated
Comment thread opentelemetry/proto/collector/profiles/v1development/profiles_service.proto Outdated
@dmathieu dmathieu force-pushed the profiles-reference-table branch from 2617db9 to 7acced1 Compare May 12, 2025 07:10
@dmathieu dmathieu changed the title Move the profiles lookup tables into a new ProfilesReferenceTable message Move the profiles lookup tables into a new Dictionary message May 13, 2025
@reyang reyang merged commit f7676e8 into open-telemetry:main May 15, 2025
15 checks passed
@dmathieu dmathieu deleted the profiles-reference-table branch May 15, 2025 15:46
@PaulinaParangerHr
Copy link
Copy Markdown

Really

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.