-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[pkg/ottl] Add support for non maps elements to SliceToMap #43521
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
base: main
Are you sure you want to change the base?
[pkg/ottl] Add support for non maps elements to SliceToMap #43521
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
bc159a3 to
5dd8558
Compare
5dd8558 to
1261caf
Compare
|
@edmocosta could you please allow the workflow on this PR? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RealAnna! Looks good so far.
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
0f75296 to
1fb115b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, thanks for implementing it @RealAnna! I've left a few suggestions for your consideration.
| elem := v.At(i) | ||
|
|
||
| // If key_path is not set, key is the index | ||
| key := strconv.Itoa(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's only execute strconv.Itoa(i) if necessary and save some cycles on critical/busy paths, we can move this assignment down to the if useKeyPath condition.
| } | ||
| extractedValue, err := extractValue(e, valuePath.Get()) | ||
|
|
||
| err := putValue(value, m, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The putValue function is probably not needed, we can replace it by:
value.CopyTo(m.PutEmpty(key))Co-authored-by: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com>
Co-authored-by: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com>
Description
Supports non Map elements
Link to tracking issue
Fixes #43099
Testing
added a few corresponding table tests