[pkg/ottl]: Create ctxprofilecommon for common attribute handling#42107
Conversation
florianl
requested review from
a team,
TylerHelmuth,
edmocosta and
evan-bradley
as code owners
…various profiling sub messages Most Profiling messages do have some attributes. Create ctxprofilecommon for shared functionality. Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
florianl
force-pushed
the
ctxprofilecommon
branch
from
c23786d to
69c7ebd
Compare
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
edmocosta
left a comment
edmocosta
left a comment
There was a problem hiding this comment.
Thank you for doing this!
It seems this PR has introduced more changes than #41814 (comment) suggested, which is fine, but I would recommend keeping it at the minimum for now, and only tackle those two functions that implements the shared profile set attribute logic, so we can focus and move other profiles PRs faster.
Unit tests are also failing, which maybe means we broke some existing behavior.
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
@edmocosta thanks for the feedback. When testing the changes locally, I did skip the e2e tests as I was not aware of them. So having them fail, uncovered already an issue and differences in the attribute handling between ctxprofile and ctxprofilesample. I managed to fix this now in ctxprofilecommon and so now we can also be sure, that attribute handling is always the same across all profile sub messages. |
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
Merge conflicts with recent protocol changes are resolved now. Looking for feedback @edmocosta @TylerHelmuth @evan-bradley |
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
edmocosta
changed the title
4 participants
Description
Most Profiling messages do have some attributes. Create ctxprofilecommon for shared functionality.
Follow up to #41814 (comment)
Link to tracking issue
Fixes
Testing
Documentation