opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

Support IsMatch function take pattern parameter from attributes

Open rtekdogan opened this issue 2 years ago • 7 comments
trafficstars

Component(s)

No response

Is your feature request related to a problem? Please describe.

While collecting stdout logs, we would like to filter some containers based on pod annotations. With current filterprocessor and static string can be used as pattern, I request to IsMatch function to accept attributes as second parameter

Describe the solution you'd like

I suggested my solution with this PR

Describe alternatives you've considered

No response

Additional context

No response

rtekdogan avatar Nov 07 '23 20:11 rtekdogan

Pinging code owners for pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley. See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] avatar Nov 07 '23 21:11 github-actions[bot]

@evan-bradley we've had a "standard" (not documented anywhere) that patterns in OTTL functions be static. So far this has provided a couple things:

  • regex validation during startup
  • building the regex once and reusing it, saving some performance (theoretically).

If we update the pattern to allow dynamic value we'll loose those features even when using static strings (unless we get fancy, which maybe we can).

I've always imagined we'd have a use case where the pattern needs to come from an attribute and it has now come up. I am ok allowing dynamic values, and if we do, I'd like to allow the pattern for all functions that need them.

TylerHelmuth avatar Nov 13 '23 18:11 TylerHelmuth

I'd also like to allow providing dynamic values in any function arguments that hold arbitrary strings. We definitely can find a way to keep our current functionality for static-value arguments, but I think we will need to add to the language to enable that.

evan-bradley avatar Nov 14 '23 21:11 evan-bradley

I wonder if we could add to the Getter interface some sort of IsStatic function that helps identify this Getter as returning a static value.

TylerHelmuth avatar Nov 14 '23 22:11 TylerHelmuth

Is there any update about this issue?

rtekdogan avatar Dec 07 '23 08:12 rtekdogan

If you can come up with a way to perform regex validation of static strings when the pattern parameter is a StringGetter that would be the preferred option, but I'm willing to move to a StringGetter even if that isn't possible.

Please add some benchmarks to the function so we can compare the performance before and after.

TylerHelmuth avatar Dec 07 '23 15:12 TylerHelmuth

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] avatar Feb 06 '24 03:02 github-actions[bot]

Hi @TylerHelmuth I wrote some benchmark test in my fork

In proposed solution, I cached compiled patterns and wrote 2 benchmark test, 1 for same pattern with same target and the other one for changing pattern in each iteration and keeping target same.

Results for proposed change:

goos: darwin goarch: arm64 pkg: github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottlfuncs Benchmark_performance_same_pattern-8 2390307 503.1 ns/op Benchmark_performance_different_pattern-8 466681 3142 ns/op PASS ok github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottlfuncs 3.950s

Results for existing:

goos: darwin goarch: arm64 pkg: github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottlfuncs Benchmark_performance_same_pattern-8 705002 1683 ns/op Benchmark_performance_different_pattern-8 610514 2195 ns/op PASS ok github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottlfuncs 3.325s

rtekdogan avatar Mar 01 '24 14:03 rtekdogan

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] avatar May 01 '24 03:05 github-actions[bot]

@rtekdogan thank you for those benchmarks. I have not had time to work on this issue yet, but I think there is still a way for a Getter to announce that it is static and can be called before hot-path execution.

TylerHelmuth avatar May 02 '24 18:05 TylerHelmuth

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

github-actions[bot] avatar Jul 03 '24 03:07 github-actions[bot]