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

[pkg/ottl] Use generics to avoid context cast in getters and funcs #14482

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Sep 26, 2022

This PR also improves number of allocations (by 2, if I am not wrong is because of the removing of the ottl.TransformContext interface use, which requires a heap allocation) and overall performance of the transform processor.

Before:

goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/processor/transformprocessor/internal/traces
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkTwoSpans
BenchmarkTwoSpans/no_processing
BenchmarkTwoSpans/no_processing-16         	  783144	      1394 ns/op	    1528 B/op	      31 allocs/op
BenchmarkTwoSpans/set_attribute
BenchmarkTwoSpans/set_attribute-16         	  713299	      1691 ns/op	    1832 B/op	      35 allocs/op
BenchmarkTwoSpans/keep_keys_attribute
BenchmarkTwoSpans/keep_keys_attribute-16   	  789517	      1626 ns/op	    1560 B/op	      33 allocs/op
BenchmarkTwoSpans/no_match
BenchmarkTwoSpans/no_match-16              	  799210	      1520 ns/op	    1560 B/op	      33 allocs/op
BenchmarkTwoSpans/inner_field
BenchmarkTwoSpans/inner_field-16           	  737241	      1566 ns/op	    1560 B/op	      33 allocs/op
BenchmarkTwoSpans/inner_field_both_spans
BenchmarkTwoSpans/inner_field_both_spans-16         	  763725	      1622 ns/op	    1592 B/op	      35 allocs/op

After:

goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/processor/transformprocessor/internal/traces
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkTwoSpans
BenchmarkTwoSpans/no_processing
BenchmarkTwoSpans/no_processing-16         	  868864	      1296 ns/op	    1480 B/op	      29 allocs/op
BenchmarkTwoSpans/set_attribute
BenchmarkTwoSpans/set_attribute-16         	  738514	      1609 ns/op	    1784 B/op	      33 allocs/op
BenchmarkTwoSpans/keep_keys_attribute
BenchmarkTwoSpans/keep_keys_attribute-16   	  800833	      1450 ns/op	    1512 B/op	      31 allocs/op
BenchmarkTwoSpans/no_match
BenchmarkTwoSpans/no_match-16              	  835160	      1394 ns/op	    1512 B/op	      31 allocs/op
BenchmarkTwoSpans/inner_field
BenchmarkTwoSpans/inner_field-16           	  796388	      1445 ns/op	    1512 B/op	      31 allocs/op
BenchmarkTwoSpans/inner_field_both_spans
BenchmarkTwoSpans/inner_field_both_spans-16         	  798840	      1539 ns/op	    1544 B/op	      33 allocs/op

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I'll do an in-depth review over the next week, but no feedback so far.

pkg/ottl/expression.go Outdated Show resolved Hide resolved
processor/transformprocessor/config.go Outdated Show resolved Hide resolved
if err != nil {
errors = multierr.Append(errors, err)
}

ottlp = ottl.NewParser(
ottllogsp := ottl.NewParser[ottllogs.TransformContext](
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a followup change, but instead of a generic NewParser, should contexts provide a Parser specific to their context? Something like ottllogs.NewParser(logs.Functions(), component.TelemetrySettings{})?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that more, since 2 arguments are always coming from that package. Will address it in the next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, do you plan to work on that or do we need an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do after you approve this 🗡️

@@ -26,7 +26,7 @@ import (
)

type Processor struct {
queries []ottl.Statement
queries []ottl.Statement[ottllogs.TransformContext]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could do something similar here as proposed for NewParser? Have []ottllogs.Statements so that we don't have to have the processor pass in the Generic itself.

Hopefully we can make it so that the only place a processor would need to directly interact with the generic is when creating its own functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

So essentially have a TracesStatements?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can also look into this for the next PR when I look into the Parser.

Copy link
Member

Choose a reason for hiding this comment

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

Ya something like that. Since the generic introduce some complexity, will be great to hide that complexity inside the Contexts as much as possible since, at least at the moment, we need a Parser and set of Statements per context.

@TylerHelmuth
Copy link
Member

@bogdandrutu I'll give this a final look over tomorrow.

@bogdandrutu bogdandrutu force-pushed the generics branch 3 times, most recently from 793114f to e0db77d Compare September 28, 2022 20:56
…14338)

[processor/transform] Added the Int function to the transform processor (#11810)
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice use of generics!

@bogdandrutu bogdandrutu merged commit e4695ee into open-telemetry:main Sep 29, 2022
@bogdandrutu bogdandrutu deleted the generics branch September 29, 2022 17:09
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants