Remove lookup table access from profiles in ottl #40227
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
requested review from
edmocosta,
evan-bradley and
TylerHelmuth
rockdaboot
approved these changes
May 26, 2025
edmocosta
reviewed
May 26, 2025
| case "location_indices": | ||
| return accessLocationIndices[K](), nil | ||
| case "function_table": | ||
| return accessFunctionTable[K](), nil | ||
| case "attribute_table": |
There was a problem hiding this comment.
Should we also remove the attribute_indices? Not sure if those indices would be useful without this removed table. Same question applies to other *indices that the lookup table is being removed: location_indices, comment_string_indices, default_sample_type_string_index (seems to use the string_table).
If the plan is to abstract the internal format, I guess we wouldn't want them to be set manually, and some mechanism similar to the attributes would be provided. Is that correct?
There was a problem hiding this comment.
The plan is to make the pdata upgrade possible.
There was a problem hiding this comment.
I agree removing the _indices fields makes sense as well, and I'm happy to do it. But I'd rather do that in another PR in order to avoid stalling #40285
Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
edmocosta
approved these changes
May 26, 2025
atoulme
approved these changes
May 27, 2025
mx-psi
pushed a commit
that referenced
this pull request
Jun 2, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR goes in sync with open-telemetry/opentelemetry-collector#13075, and upgrades pdata to 1.7.0. This is currently based after #40227. It can either come together or separately from that other PR. This has been tested locally. CI can't pass now because it relies on the changes in the core PR, which haven't been merged yet.
dd-jasminesun
pushed a commit
to DataDog/opentelemetry-collector-contrib
that referenced
this pull request
Jun 23, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Access to these lookup tables isn't really useful, as they only provide a slice of values, and we can't check which one is being used by the current profile. Also, with open-telemetry/opentelemetry-collector#13075, the lookup tables are moving out of the profile and into a new dictionary object. So as a first step to the proto migration, this removes access to the lookup tables for a profile. The replacement for this is open-telemetry#39681, which will give acces to the profile attributes, as we do with other signals and abstract away the lookup tables. --------- Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Access to these lookup tables isn't really useful, as they only provide a slice of values, and we can't check which one is being used by the current profile.
Also, with open-telemetry/opentelemetry-collector#13075, the lookup tables are moving out of the profile and into a new dictionary object.
So as a first step to the proto migration, this removes access to the lookup tables for a profile.
The replacement for this is #39681, which will give acces to the profile attributes, as we do with other signals and abstract away the lookup tables.