[pkg/ottl] Fix OTTL functions by using setters

[rockdaboot]

Description

Some OTTL functions do not call the accessor.setter function, but instead change the variable directly.
This doesn't work with the profiles signal where non-trivial processing is needed.

For example, attributes are split into a lookup table and arrays of indices (the rationale is de-duplication). While the accessor.getter function returns a pcommon.Map for attributes, any changes made to that map require passing the map to the accessor.setter function.

Link to tracking issue

Fixes #39100

Testing

Documentation

[rockdaboot]

@TylerHelmuth Can you please also review #39657, which became a requirement for this PR?

[github-actions]

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

[github-actions]

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

[rockdaboot]

The existing e2e tests give me confidence for all the non-profiles context. Would it be worth adding e2e tests for profiles specifically since it behaves different that the other contexts? Right now the e2e tests only use ottllog since they all behaved in the same general way.

For useful profiles based e2e tests, we first should merge #39681, because the current e2e tests heavily rely on attributes accessors. Without attributes processors, e2e test would look completely different to the logs based e2e tests. That's a) hardly to compare and b) lot of work and c) more code to maintain (no de-duplication of code possible).

[edmocosta]

For useful profiles based e2e tests, we first should merge #39681, because the current e2e tests heavily rely on attributes accessors. Without attributes processors, e2e test would look completely different to the logs based e2e tests. That's a) hardly to compare and b) lot of work and c) more code to maintain (no de-duplication of code possible).

I agree it would have more value testing the editors with the paths that makes the profile context different, such as the attributes, and I'd be okay handling it in a separate issue(#40738).

Before moving forward with this one, I really think we should add a few more tests cases to ensure that the problem this PR fixes won't happen again. We can do that by verifying that the setter function was actually called, and that the result is correct not just because the editor is directly modifying the object reference, but also because of setter call.

[rockdaboot]

For useful profiles based e2e tests, we first should merge #39681, because the current e2e tests heavily rely on attributes accessors. Without attributes processors, e2e test would look completely different to the logs based e2e tests. That's a) hardly to compare and b) lot of work and c) more code to maintain (no de-duplication of code possible).

I agree it would have more value testing the editors with the paths that makes the profile context different, such as the attributes, and I'd be okay handling it in a separate issue(#40738).

Before moving forward with this one, I really think we should add a few more tests cases to ensure that the problem this PR fixes won't happen again. We can do that by verifying that the setter function was actually called, and that the result is correct not just because the editor is directly modifying the object reference, but also because of setter call.

Added a check that the setter got actually called. This still doesn't check whether the editor was directly modifying the object as well. @edmocosta Would is the preferred way of doing that?

[edmocosta]

Added a check that the setter got actually called. This still doesn't check whether the editor was directly modifying the object as well. @edmocosta Would is the preferred way of doing that?

We currently don't have any efficient mechanism to enforce the immutability of those returned maps - and my PData PR got some pushback - so I think it would be fine to keep it as it's for now, and tackle it later when we decide how to deal with that in the OTTL core.

@TylerHelmuth please let me know if you agree with this, and with moving this PR forward.

Thanks!

[rockdaboot]

@edmocosta @TylerHelmuth We now have OTTL e2e tests based on profiles.
The tests uncovered an issue in accessAttributesKey(), which is fixed in this PR.
Please let me know what you think.