[pkg/telemetryquerylanguage] add replace_key_all_patterns function

[fatsheep9146]

Signed-off-by: Ziqi Zhao [email protected]

Description:
fix #12631

[TylerHelmuth]

@fatsheep9146 can you break this into 2 PR, one for the function add and one to the transform processor? It'd be nice to do it all in one PR but any functions added to transformprocessor should have integration tests added to a signals processor_test.go and I think the PR would be too large.

[fatsheep9146]

@fatsheep9146 can you break this into 2 PR, one for the function add and one to the transform processor? It'd be nice to do it all in one PR but any functions added to transformprocessor should have integration tests added to a signals processor_test.go and I think the PR would be too large.

I've removed it from transform processor, and I will submit second PR until this one is merged.

[fatsheep9146]

The integration test seems unrelated to this pr @TylerHelmuth

[TylerHelmuth]

@bogdandrutu @kentquirk curious on your thoughts on this function. It will certainly meet the needs of the user to rename keys in bulk, but we'd end up with replace_match, replace_all_matches, replace_pattern, replace_all_patterns, and replace_key_all_patterns functions, all that are named similarly and do similar, but slightly different things.

For this particular use case we could instead update replace_all_patterns to take a new parameter (string or enum) that specifies whether the function applies to keys or values and update the logic accordingly. This would be a breaking change (which is ok bc the processor/tql is still in alpha), but would keep the number of functions lower.

[TylerHelmuth]

For this particular use case we could instead update replace_all_patterns to take a new parameter (string or enum) that specifies whether the function applies to keys or values and update the logic accordingly.

@fatsheep9146 please update your implementation to match this comment instead of adding a new function.

[fatsheep9146]

For this particular use case we could instead update replace_all_patterns to take a new parameter (string or enum) that specifies whether the function applies to keys or values and update the logic accordingly.

@fatsheep9146 please update your implementation to match this comment instead of adding a new function.

I already add a new parameter, please help review this =D @TylerHelmuth

[fatsheep9146]

Please help review this again =D @TylerHelmuth

[fatsheep9146]

All comments are fixed. @TylerHelmuth

[TylerHelmuth]

Thanks for doing this!

[fatsheep9146]

@bogdandrutu Could you help review this?

[kentquirk]

This seems correct as is, but I wonder if it's worth trying to avoid some allocations in the general case.

[kentquirk]

I think it will be a fairly common occurrence that this function does no useful work -- that none of the attributes match the given pattern. Consequently, it might make sense, then, to do one of two things:

• set a flag when the match succeeds, and only do the target.Set if it has succeeded at least once; this avoids rewriting target, although it doesn't avoid the allocation
• do a pre-pass over the keys looking for a match, and if none is found exit without allocating the updated value

Is it worth it?

[TylerHelmuth]

We haven't done serious benchmarks for OTTl or the transform processor yet, but we should. As both mature I say we should take performance improvements wherever we can find them.

[TylerHelmuth]

Although I think the suggestion can be done in a future PR if you open an issue to call it out.

I think I'd also like to merge all the Replace functions into one.

[TylerHelmuth]

@fatsheep9146 please address conflicts

[fatsheep9146]

@fatsheep9146 please address conflicts

I think the failed task is due to transformprocessor

make[2]: Entering directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/processor/transformprocessor'
Check License finished successfully
running misspell -error
golangci-lint run --allow-parallel-runners
make[2]: Leaving directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/processor/tailsamplingprocessor'
Error: internal/logs/processor_test.go:90:79: td.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().InsertString undefined (type pcommon.Map has no field or method InsertString) (typecheck)
				td.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().InsertString("url", "http://localhost/health")

Should I fix those in seperate prs? @TylerHelmuth

[TylerHelmuth]

@fatsheep9146 Ya those errors don't seem correct. Attempt to fix in another PR, but I suspect if you create a new branch from main you won't see them.

[fatsheep9146]

@fatsheep9146 Ya those errors don't seem correct. Attempt to fix in another PR, but I suspect if you create a new branch from main you won't see them.

Sorry, it's my mistake, the reason is that in transformprocessor test, I also fix some test for this feature, I will fix this.

[codeboten]

Needs a rebase

[fatsheep9146]

Needs a rebase

Rebase done, I think this is ready-to-merge. @codeboten @TylerHelmuth

[fatsheep9146]

@TylerHelmuth @codeboten Could you help add ready-to-merge label?

[bogdandrutu]

Please add a changelog entry.

[TylerHelmuth]

@fatsheep9146 please rebase.

[EddieEldridge]

This is awesome, thanks so much everyone 🍾