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

[processor/transform] Extract function arguments into separate object… #8339

Merged
merged 2 commits into from
Mar 24, 2022

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Mar 8, 2022

… for easier extension

Description: Extracts arguments passed to transform functions into a struct for easy extension

@Aneurysm9 had originally had this idea and initially I went with simpler listing out the arguments to avoid any performance issues. Because we call functions through function pointers, Go can't really optimize the struct away. That being said, being able to extend the data that is available to a transform function also seems important, for example I heard about a case to provide auth credentials information to transformation. So I want to see what people think of this style.

The benchmarks are consistently slower but for 100 spans ranges from 2-15us or so, getting consistent numbers at this scale is difficult so it seems to still be within noise to some extent. Let me know thoughts.

Link to tracking Issue: #7300

Testing: Unit tests

Documentation:

After

BenchmarkTwoSpans/no_processing-16                777204              1487 ns/op
BenchmarkTwoSpans/set_attribute-16                720290              1669 ns/op
BenchmarkTwoSpans/keep_keys_attribute-16          654525              1865 ns/op
BenchmarkTwoSpans/no_match-16                     749661              1600 ns/op
BenchmarkTwoSpans/inner_field-16                  724689              1667 ns/op
BenchmarkTwoSpans/inner_field_both_spans-16               696908              1742 ns/op
BenchmarkHundredSpans/no_processing-16                     25939             46176 ns/op
BenchmarkHundredSpans/set_status_code-16                   21024             57097 ns/op
BenchmarkHundredSpans/hundred_queries-16                    2462            497112 ns/op

Before

BenchmarkTwoSpans/no_processing-16                766363              1497 ns/op
BenchmarkTwoSpans/set_attribute-16                712767              1675 ns/op
BenchmarkTwoSpans/keep_keys_attribute-16          647737              1885 ns/op
BenchmarkTwoSpans/no_match-16                     756135              1596 ns/op
BenchmarkTwoSpans/inner_field-16                  730369              1661 ns/op
BenchmarkTwoSpans/inner_field_both_spans-16               690099              1735 ns/op
BenchmarkHundredSpans/no_processing-16                     25314             46236 ns/op
BenchmarkHundredSpans/set_status_code-16                   21282             56543 ns/op
BenchmarkHundredSpans/hundred_queries-16                    2520            473097 ns/op

@anuraaga anuraaga requested a review from a team March 8, 2022 07:32
@anuraaga anuraaga requested a review from bogdandrutu as a code owner March 8, 2022 07:32
@anuraaga anuraaga requested a review from Aneurysm9 March 8, 2022 08:05
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Would be nice to include before/after benchmarks in the description for posterity, but this LGTM.

@Aneurysm9 Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Mar 24, 2022
@jpkrohling jpkrohling added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 24, 2022
@jpkrohling jpkrohling merged commit 2324192 into open-telemetry:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants