Skip to content

[pkg/ottl] Accessors for profile attributes#39681

Merged
evan-bradley merged 2 commits into
open-telemetry:mainfrom
rockdaboot:profiles-attribute-accessors
Jun 23, 2025
Merged

[pkg/ottl] Accessors for profile attributes#39681
evan-bradley merged 2 commits into
open-telemetry:mainfrom
rockdaboot:profiles-attribute-accessors

Conversation

@rockdaboot
Copy link
Copy Markdown
Contributor

@rockdaboot rockdaboot commented Apr 28, 2025

Description

Adds support for profile.attributes to OTTL.

This allows users to use profile.attributes without deeper knowledge of the protocol internals.
Currently, users have to deal with attribute_table and attribute_indices, which is error-prone and fragile when the experimental profiling signal changes internal representation.

This change depends on open-telemetry/opentelemetry-collector#12798 and thus includes that PR. It will be removed as soon as possible.

The profiles support for the transformprocessor currently relies/waits on this change.

@github-actions github-actions Bot requested a review from kentquirk April 28, 2025 08:20
@rockdaboot rockdaboot marked this pull request as draft April 28, 2025 08:20
@rockdaboot rockdaboot force-pushed the profiles-attribute-accessors branch from 3d6044b to ecc282f Compare April 28, 2025 08:23
@rockdaboot rockdaboot changed the title [chore] [pkg/ottl] Accessors for profile attributes [pkg/ottl] Accessors for profile attributes Apr 28, 2025
@rockdaboot rockdaboot force-pushed the profiles-attribute-accessors branch from ecc282f to 6745566 Compare April 29, 2025 07:32
@rockdaboot rockdaboot marked this pull request as ready for review April 29, 2025 07:33
Comment thread pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
Comment thread pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
Comment thread pkg/ottl/contexts/internal/ctxprofile/profile.go Outdated
Comment thread pkg/ottl/contexts/internal/ctxprofile/profile_test.go
@edmocosta edmocosta marked this pull request as draft May 5, 2025 15:28
@edmocosta
Copy link
Copy Markdown
Contributor

edmocosta commented May 5, 2025

Please mark it as ready for review when open-telemetry/opentelemetry-collector#12798 gets merged. Thanks.

@github-actions
Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@rockdaboot
Copy link
Copy Markdown
Contributor Author

@edmocosta The pdata PutAttribute function now has been merged and adopted in this PR.

I'd still wait for #39952 until marking this PR as ready to review.

atoulme pushed a commit that referenced this pull request May 27, 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
#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 <11836452+edmocosta@users.noreply.github.com>
andrzej-stencel pushed a commit that referenced this pull request Jun 9, 2025
…yName()` internal functions (#39952)

#### Description
These functions have been requested
[here](#39681 (comment))
and
[here](#39681 (comment))
as prerequisite for OTTL attributes accessors for the profiles signal.

Also adds tests for `GetMap()`, that were missing.

#### Testing
Unit tests

---------

Co-authored-by: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com>
@edmocosta
Copy link
Copy Markdown
Contributor

Hi @rockdaboot, #39952 is already merged, could you please update the branch and mark it as ready for review? Thank you!

Copy link
Copy Markdown
Contributor

@edmocosta edmocosta left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks for working on that and I'm sorry for the reviewing delay.

Regarding the profile dictionary, whenever I set an attributes it creates another entry on the attributes tables and updates the profile's attributes indices, which I guess is expected as we might have the same key with different values used by different profiles. My only concern is how the transform processor, for example, will impact the size of that table, considering that it happens even when I set the same profile's attribute key. For example, the following statements would generate two profile attributes table entries:

- set(attributes["newKey"], 1)
- set(attributes["newKey"], 2)

I guess it's okay, but I'm wondering if future lower contexts, that will run it in a loop, could become an issue, as this table would grow very quickly depending on the users statements. Just a thought, I understand you cannot tackle it on this PR, but just wanted to double check this behavior.

@rockdaboot
Copy link
Copy Markdown
Contributor Author

rockdaboot commented Jun 18, 2025

It looks good to me, thanks for working on that and I'm sorry for the reviewing delay.

Regarding the profile dictionary, whenever I set an attributes it created another entry on the attributes tables and updates the profile's attributes indices, which I guess is expected as we might have the same key with different values used by different profiles. My only concern is how the transform processor, for example, will impact the size of that table, considering that it happens even when I set the same profile's attribute key. For example, the following statements would generate two profile attributes table entries:

- set(attributes["newKey"], 1)
- set(attributes["newKey"], 2)

I guess it's okay, but I'm wondering if future lower contexts, that will run it in a loop, could become an issue, as this table would grow very quickly depending on the users statements. Just a thought, I understand you cannot tackle it on this PR, but just wanted to double check this behavior.

Yes, that is expected atm. The table is a set of (unique) k/v pairs.
To automatically clean up unused entries, we would need something like a reference counter (and then re-use table entries with no reference).
Or as an expensive alternative, walk through all attribute index arrays to re-create the table.

But nothing we can tackle in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants