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

[internal/filter] Add bridge from filterspan to filterottl #21054

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Apr 18, 2023

Description:
This PR adds a bridge between filterspan.NewSkipExpr and filterottl.NewBoolExprForSpan behind a feature gate. With the feature gate enabled, any component using filterspan.NewSkipExpr will start using OTTL behind the scenes.

Link to tracking Issue:
Related to #18643
Related to #18642

Testing:
Before adding the feature gate, all unit tests from any component using filterspan are were testing this bridge.

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Apr 26, 2023

@dmitryax here is a quick benchmark comparison between filterspan using filtermatcher/filterset and filterspan using filterottl. I only wrote up 1 static and 1 regex case so far as I think it shows the general change in performance, but I can add more tests for more scenarios as well.

The tests against filtermatcher/filterset, were run from this branch.

filtermatcher/filterset results:

❯ go test -bench=. -run=notests --tags="" -benchmem github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
BenchmarkFilterspan_NewSkipExpr/service_name_match_regexp-10             4606274               247.7 ns/op             0 B/op          0 allocs/op
BenchmarkFilterspan_NewSkipExpr/service_name_match_static-10             6298548               189.9 ns/op             0 B/op          0 allocs/op

filterottl results

❯ go test -bench=. -run=notests --tags="" -benchmem github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
BenchmarkFilterspan_NewSkipExpr/service_name_match_regexp-10             3404667               336.1 ns/op            32 B/op          2 allocs/op
BenchmarkFilterspan_NewSkipExpr/service_name_match_static-10             4759767               252.1 ns/op            16 B/op          1 allocs/op

Comparison using benchstat

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
                                                    │   old.txt   │               new.txt               │
                                                    │   sec/op    │   sec/op     vs base                │
Filterspan_NewSkipExpr/service_name_match_regexp-10   258.8n ± 1%   354.0n ± 0%  +36.79% (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   199.4n ± 0%   262.7n ± 4%  +31.78% (p=0.000 n=10)
geomean                                               227.1n        305.0n       +34.26%

                                                    │   old.txt   │           new.txt            │
                                                    │    B/op     │    B/op     vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.00 ± 0%     32.00 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.00 ± 0%     16.00 ± 0%  ? (p=0.000 n=10)
geomean                                                         ¹   22.63       ?
¹ summaries must be >0 to compute geomean

                                                    │   old.txt    │           new.txt            │
                                                    │  allocs/op   │ allocs/op   vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.000 ± 0%     2.000 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.000 ± 0%     1.000 ± 0%  ? (p=0.000 n=10)
geomean                                                          ¹   1.414       ?
¹ summaries must be >0 to compute geomean

The results show we do take a performance hit using OTTL. Both cases gain an allocation when grabbing a value from the resource attributes and the regex case gets an extra allocation since it uses StringLikeGetter to try and convert the value to a string.

Personally I think the hit to performance is worth the flexibility and maintainability we gain by centralizing filtering around OTTL. @open-telemetry/collector-contrib-approvers what do you think?

@TylerHelmuth TylerHelmuth force-pushed the filter-to-filterottl-bridge branch 2 times, most recently from 238e6f5 to ab76ac7 Compare May 1, 2023 17:57
@TylerHelmuth
Copy link
Member Author

With the new feature allowing converters to be used as constant booleans (#20911), we have narrowed the performance gap slightly.

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
                                                    │   old.txt   │               new.txt               │
                                                    │   sec/op    │   sec/op     vs base                │
Filterspan_NewSkipExpr/service_name_match_regexp-10   220.3n ± 1%   292.4n ± 1%  +32.75% (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   175.5n ± 0%   223.4n ± 0%  +27.29% (p=0.000 n=10)
geomean                                               196.6n        255.6n       +29.99%

                                                    │   old.txt   │           new.txt            │
                                                    │    B/op     │    B/op     vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.00 ± 0%     32.00 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.00 ± 0%     16.00 ± 0%  ? (p=0.000 n=10)
geomean                                                         ¹   22.63       ?
¹ summaries must be >0 to compute geomean

                                                    │   old.txt    │           new.txt            │
                                                    │  allocs/op   │ allocs/op   vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.000 ± 0%     2.000 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.000 ± 0%     1.000 ± 0%  ? (p=0.000 n=10)
geomean                                                          ¹   1.414       ?
¹ summaries must be >0 to compute geomean

The current implementation doesn't check if it needs to use () for groupings, it uses them whenever they could possibly be needed. I will experiment and see if writing the most efficient statement (least amount of characters), results in the most performant statement due to the decrease in the boolean expression chain.

@TylerHelmuth
Copy link
Member Author

Continued increase from removing unneeded parentheses:

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterspan
                                                    │   old.txt   │               new.txt               │
                                                    │   sec/op    │   sec/op     vs base                │
Filterspan_NewSkipExpr/service_name_match_regexp-10   223.1n ± 1%   290.9n ± 1%  +30.37% (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   176.2n ± 1%   222.1n ± 0%  +26.05% (p=0.000 n=10)
geomean                                               198.3n        254.2n       +28.19%

                                                    │   old.txt   │           new.txt            │
                                                    │    B/op     │    B/op     vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.00 ± 0%     32.00 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.00 ± 0%     16.00 ± 0%  ? (p=0.000 n=10)
geomean                                                         ¹   22.63       ?
¹ summaries must be >0 to compute geomean

                                                    │   old.txt    │           new.txt            │
                                                    │  allocs/op   │ allocs/op   vs base          │
Filterspan_NewSkipExpr/service_name_match_regexp-10   0.000 ± 0%     2.000 ± 0%  ? (p=0.000 n=10)
Filterspan_NewSkipExpr/service_name_match_static-10   0.000 ± 0%     1.000 ± 0%  ? (p=0.000 n=10)
geomean                                                          ¹   1.414       ?
¹ summaries must be >0 to compute geomean

@TylerHelmuth TylerHelmuth force-pushed the filter-to-filterottl-bridge branch 4 times, most recently from 5af6615 to 79b2f84 Compare May 18, 2023 18:32
@TylerHelmuth TylerHelmuth merged commit c527b95 into open-telemetry:main May 25, 2023
85 checks passed
@TylerHelmuth TylerHelmuth deleted the filter-to-filterottl-bridge branch May 25, 2023 17:11
@github-actions github-actions bot added this to the next release milestone May 25, 2023
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.

None yet

2 participants