[pkg/ottl] Fix OTTL functions by using setters #39416
Conversation
github-actions
bot
requested review from
TylerHelmuth,
bogdandrutu,
edmocosta,
evan-bradley and
kentquirk
rockdaboot
force-pushed
the
fix-39100
branch
4 times, most recently
from
13994d2 to
83fe3d3
Compare
rockdaboot
changed the title
rockdaboot
force-pushed
the
fix-39100
branch
2 times, most recently
from
0ac9801 to
6a2fc36
Compare
rockdaboot
force-pushed
the
fix-39100
branch
3 times, most recently
from
2584f30 to
409957f
Compare
|
@TylerHelmuth Can you please also review #39657, which became a requirement for this PR? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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 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? |
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! |
|
@edmocosta @TylerHelmuth We now have OTTL e2e tests based on profiles. |
#### Description Introduce OTTL e2e tests based on profiles. The tests uncovered an issue in accessAttributesKey(), which is fixed in this PR. The e2e test have been requested [here](#39416 (comment)) by @TylerHelmuth. #### Link to tracking issue Fixes #40738 #### THIS PR WILL ONLY WORK ON TOP OF #39416. --------- Co-authored-by: Tyler Helmuth <[email protected]> Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
#### Description PR for profiles support in the transform processor. It's a split-out from #39036 and contains only basic functionality, that makes this PR independent from #39681, #39416 and open-telemetry/opentelemetry-collector#12798. For this, several tests were commented out. The reason for this PR is the hope to get it merged so that users and developers can start experimenting with profiles functionality in the transformprocessor. Attributes can't currently can't be used with profiles (due to the above mentioned unmerged PRs). #### Link to tracking issue Fixes #39009 --------- Co-authored-by: Edmo Vamerlatti Costa <[email protected]> Co-authored-by: Tyler Helmuth <[email protected]>
#### 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 open-telemetry#39100 <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.-->
#### Description Introduce OTTL e2e tests based on profiles. The tests uncovered an issue in accessAttributesKey(), which is fixed in this PR. The e2e test have been requested [here](open-telemetry#39416 (comment)) by @TylerHelmuth. #### Link to tracking issue Fixes open-telemetry#40738 #### THIS PR WILL ONLY WORK ON TOP OF open-telemetry#39416. --------- Co-authored-by: Tyler Helmuth <[email protected]> Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
#### Description PR for profiles support in the transform processor. It's a split-out from open-telemetry#39036 and contains only basic functionality, that makes this PR independent from open-telemetry#39681, open-telemetry#39416 and open-telemetry/opentelemetry-collector#12798. For this, several tests were commented out. The reason for this PR is the hope to get it merged so that users and developers can start experimenting with profiles functionality in the transformprocessor. Attributes can't currently can't be used with profiles (due to the above mentioned unmerged PRs). #### Link to tracking issue Fixes open-telemetry#39009 --------- Co-authored-by: Edmo Vamerlatti Costa <[email protected]> Co-authored-by: Tyler Helmuth <[email protected]>
5 participants
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