Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] adapt mapGetter to handle nested map items within slices #37408

Merged
merged 39 commits into from
Mar 12, 2025

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Jan 22, 2025

Description

This PR fixes the limitations described in #37405. The recently introduced ValueExpression had to be adapted, and will now only return raw types, instead of their pcommon.Map/pcommon.Slice eqivalents, as suggested in #37280 (comment)

Link to tracking issue

Fixes #37405

Testing

Adapted and extended the unit and e2e tests

… pcommon types

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review January 22, 2025 09:57
@bacherfl bacherfl requested a review from a team as a code owner January 22, 2025 09:57
@bacherfl
Copy link
Contributor Author

Converting back to draft for now, due to #37280 (comment)

@bacherfl bacherfl marked this pull request as draft January 23, 2025 07:28
…er instead

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review January 23, 2025 10:49
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl changed the title [pkg/ottl] adapt mapGetter to return raw maps [pkg/ottl] adapt mapGetter to handle nested map items within slices Jan 23, 2025
@@ -480,6 +481,19 @@ func (p *Parser[K]) ParseValueExpression(raw string) (*ValueExpression[K], error
}

return &ValueExpression[K]{
getter: getter,
getter: &StandardGetSetter[K]{
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up stick to this approach, considering the mapGetter is now returning pcommon.Map values, I think we could keep it consistent and remove these changes, so all maps would still be parsed aspcommon.Map instead of raw values. I guess it would be a breaking change, though. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine with that, however there are also arguments for returning the raw types here (#37280 (comment)). I think I would also prefer to consistently return pcommon.Map values though. @evan-bradley @TylerHelmuth WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Judging by #37280 (comment), we probably need to survey this a bit more to make sure we're either consistent everywhere, or can translate at any given point in the chain. I'm not sure I have a strong opinion here, but it sounds like at a minimum we could potentially use more tests.

Do the same issues apply to slices with []any vs. pcommon.Slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for slices, this issue doesn't apply, as the listGetter returns []any as opposed to pcommon.Slice:

func (l *listGetter[K]) Get(ctx context.Context, tCtx K) (any, error) {
evaluated := make([]any, len(l.slice))
for i, v := range l.slice {
val, err := v.Get(ctx, tCtx)
if err != nil {
return nil, err
}
evaluated[i] = val
}
return evaluated, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @edmocosta that we should continue returning pcommon.Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I now changed it again to return pcommon.Map

bacherfl added 4 commits March 6, 2025 13:08
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
TylerHelmuth
TylerHelmuth previously approved these changes Mar 6, 2025
@atoulme
Copy link
Contributor

atoulme commented Mar 7, 2025

This looks about ready to merge. I think it'd be great to make sure @evan-bradley can review one more time.

@edmocosta
Copy link
Contributor

Before merging it, I think we still need to address this #37408 (comment), if @evan-bradley agrees, we would need to undo those changes to keep returning pcommon.Map.

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@atoulme
Copy link
Contributor

atoulme commented Mar 12, 2025

@evan-bradley please review and merge if ok?

@TylerHelmuth TylerHelmuth merged commit ccc3e6e into open-telemetry:main Mar 12, 2025
171 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Map literals within slices can not be parsed
7 participants