[pkg/ottl] Add ToKeyValueString Converter #35409
Conversation
kuiperda
requested review from
TylerHelmuth,
bogdandrutu,
evan-bradley and
a team
as code owners
There was a problem hiding this comment.
I like the round trip tests. This looks good to me. cc @TylerHelmuth
kuiperda
force-pushed
the
feat/marshalkv
branch
from
f2e9f4a
to
21b464c
Compare
|
@TylerHelmuth Thanks for the feedback, I believe I've addressed everything. |
kuiperda
changed the title
| for _, kv := range keyValues { | ||
| k := escapeAndQuoteKV(kv.key, delimiter, pairDelimiter) | ||
| vStr := escapeAndQuoteKV(kv.val.AsString(), delimiter, pairDelimiter) | ||
| kvStrings = append(kvStrings, fmt.Sprintf("%s%s%v", k, delimiter, vStr)) | ||
| } | ||
| } else { | ||
| target.Range(func(k string, v pcommon.Value) bool { | ||
| key := escapeAndQuoteKV(k, delimiter, pairDelimiter) | ||
| vStr := escapeAndQuoteKV(v.AsString(), delimiter, pairDelimiter) | ||
| kvStrings = append(kvStrings, fmt.Sprintf("%s%s%v", key, delimiter, vStr)) | ||
| return true |
There was a problem hiding this comment.
It would be nice to reuse this logic so that in the future, if we need to change it, we don't accidentally change it for only the sorted path or only the unsorted path.
| kvString := convertMapToKV(source, delimiter, pairDelimiter, sortOutput) | ||
|
|
||
| return kvString, nil |
There was a problem hiding this comment.
| kvString := convertMapToKV(source, delimiter, pairDelimiter, sortOutput) | |
| return kvString, nil | |
| return convertMapToKV(source, delimiter, pairDelimiter, sortOutput), nil |
| for _, kv := range keyValues { | ||
| k := escapeAndQuoteKV(kv.key, delimiter, pairDelimiter) | ||
| vStr := escapeAndQuoteKV(kv.val.AsString(), delimiter, pairDelimiter) | ||
| kvStrings = append(kvStrings, fmt.Sprintf("%s%s%v", k, delimiter, vStr)) |
There was a problem hiding this comment.
I'm recalling something from my past that fmt.Sprintf has to do an extra allocation to convert types. It may be more efficient, although more verbose, to handle OTTL's supported types to string manually.
There was a problem hiding this comment.
I'm noticing those values are all strings anyway and can just be joined together.
kuiperda
force-pushed
the
feat/marshalkv
branch
from
21b464c
to
2aab285
Compare
|
@TylerHelmuth I've addressed the feedback from yesterday. |
Description:
Implements ToKeyValueString OTTL Converter.
Link to tracking Issue: #35334
Testing: Added unit tests and e2e
Documentation: Added