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.

Sign up for GitHub

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

Jump to bottom

[internal/filter] Add bridge from filterlog to filterottl #22789

Merged
merged 5 commits into from
Jun 21, 2023
Merged

[internal/filter] Add bridge from filterlog to filterottl #22789

merged 5 commits into from
Jun 21, 2023

Conversation

TylerHelmuth
Copy link
Member

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

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

Testing:
Added tests comparing the output of the existing config and the bridge.

@TylerHelmuth TylerHelmuth requested review from a team and codeboten May 25, 2023 21:51
@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 25, 2023
@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented May 25, 2023 •
edited
Loading

Benchmark results:

old.txt benchmarks generated from this branch

Invalid Benchmarks 
goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterlog
                                           │   old.txt   │               new.txt               │
                                           │   sec/op    │   sec/op     vs base                │
Filterlog_NewSkipExpr/body_match_regexp-10   219.0n ± 3%   282.1n ± 4%  +28.84% (p=0.000 n=10)

                                           │  old.txt  │           new.txt            │
                                           │   B/op    │    B/op     vs base          │
Filterlog_NewSkipExpr/body_match_regexp-10   0.00 ± 0%   32.00 ± 0%  ? (p=0.000 n=10)

                                           │  old.txt   │           new.txt            │
                                           │ allocs/op  │ allocs/op   vs base          │
Filterlog_NewSkipExpr/body_match_regexp-10   0.000 ± 0%   2.000 ± 0%  ? (p=0.000 n=10)

These benchmarks had an error so not all tests were run.

@TylerHelmuth TylerHelmuth force-pushed the filterlog-to-filterottl-bridge branch from 99514be to 8c0c523 Compare May 25, 2023 22:00
@TylerHelmuth TylerHelmuth mentioned this pull request May 25, 2023
Open
13 tasks
@TylerHelmuth TylerHelmuth force-pushed the filterlog-to-filterottl-bridge branch from 8c0c523 to e67a8e1 Compare May 26, 2023 16:47
@TylerHelmuth
Copy link
Member Author

Benchmarks when using body.string.

old.txt benchmarks generated from this branch

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/internal/filter/filterlog
                                               │   old.txt   │               new.txt               │
                                               │   sec/op    │   sec/op     vs base                │
Filterlog_NewSkipExpr/body_match_regexp-10       219.1n ± 2%   288.4n ± 1%  +31.68% (p=0.000 n=10)
Filterlog_NewSkipExpr/body_match_static-10       173.8n ± 0%   215.8n ± 5%  +24.14% (p=0.000 n=10)
Filterlog_NewSkipExpr/severity_number_match-10   168.2n ± 0%   211.2n ± 0%  +25.57% (p=0.000 n=10)
geomean                                          185.7n        236.0n       +27.09%

                                               │   old.txt    │            new.txt             │
                                               │     B/op     │    B/op     vs base            │
Filterlog_NewSkipExpr/body_match_regexp-10        0.00 ± 0%     32.00 ± 0%  ? (p=0.000 n=10)
Filterlog_NewSkipExpr/body_match_static-10        0.00 ± 0%     16.00 ± 0%  ? (p=0.000 n=10)
Filterlog_NewSkipExpr/severity_number_match-10   0.000 ± 0%     0.000 ± 0%  ~ (p=1.000 n=10) ¹
geomean                                                     ²               ?                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                               │   old.txt    │            new.txt             │
                                               │  allocs/op   │ allocs/op   vs base            │
Filterlog_NewSkipExpr/body_match_regexp-10       0.000 ± 0%     2.000 ± 0%  ? (p=0.000 n=10)
Filterlog_NewSkipExpr/body_match_static-10       0.000 ± 0%     1.000 ± 0%  ? (p=0.000 n=10)
Filterlog_NewSkipExpr/severity_number_match-10   0.000 ± 0%     0.000 ± 0%  ~ (p=1.000 n=10) ¹
geomean                                                     ²               ?                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 10, 2023
@TylerHelmuth TylerHelmuth added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Jun 10, 2023
MovieStoreGuy
MovieStoreGuy reviewed Jun 14, 2023
View reviewed changes
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly can't effectively review 1400+ of added code, is there any way that this can be broken down?

internal/filter/filterlog/filterlog_test.go Show resolved Hide resolved
internal/filter/filterottl/bridge.go Outdated Show resolved Hide resolved
internal/filter/filterottl/bridge.go Outdated Show resolved Hide resolved
MovieStoreGuy
MovieStoreGuy reviewed Jun 21, 2023
View reviewed changes
internal/filter/filterottl/bridge.go
severityNumberCondition *string
}

func createConditions(mp filterconfig.MatchProperties) (conditionStatements, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for moving this to a type.

MovieStoreGuy
MovieStoreGuy approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with the changes.

@codeboten codeboten merged commit f6763a0 into open-telemetry:main Jun 21, 2023
@github-actions github-actions bot added this to the next release milestone Jun 21, 2023
@TylerHelmuth TylerHelmuth deleted the filterlog-to-filterottl-bridge branch June 21, 2023 19:01
fchikwekwe pushed a commit to fchikwekwe/opentelemetry-collector-contrib that referenced this pull request Jun 23, 2023
@TylerHelmuth @fchikwekwe
[internal/filter] Add bridge from filterlog to filterottl (open-telem…
3f4e786 
…etry#22789)

This PR adds a bridge between `filterlog.NewSkipExpr` and
`filterottl.NewBoolExprForLog` behind a feature gate. With the feature
gate enabled, any component using `filterlog.NewSkipExpr` will start
using OTTL behind the scenes.

Related to
open-telemetry#18643
Related to
open-telemetry#18642

Added tests comparing the output of the existing config and the bridge.
Caleb-Hurshman pushed a commit to observIQ/opentelemetry-collector-contrib that referenced this pull request Jul 6, 2023
@TylerHelmuth @Caleb-Hurshman
[internal/filter] Add bridge from filterlog to filterottl (open-telem…
c76a9bb 
…etry#22789)

This PR adds a bridge between `filterlog.NewSkipExpr` and
`filterottl.NewBoolExprForLog` behind a feature gate. With the feature
gate enabled, any component using `filterlog.NewSkipExpr` will start
using OTTL behind the scenes.

Related to
open-telemetry#18643
Related to
open-telemetry#18642

Added tests comparing the output of the existing config and the bridge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@codeboten codeboten codeboten approved these changes

Assignees

@mx-psi mx-psi

Labels
internal/filter never stale Issues marked with this label will be never staled and automatically removed Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.81.0
Development

Successfully merging this pull request may close these issues.
4 participants
@TylerHelmuth @codeboten @MovieStoreGuy @mx-psi