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

perf(tracer): reduce overhead of attribute sanitization #4558

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Samuron
Copy link
Contributor

@Samuron Samuron commented Mar 20, 2024

Which problem is this PR solving?

OTEL is producing too much allocations

Fixes # (issue)

Short description of the changes

Added possibility for sanitizeAttributes to operate in place on provided object. Remove Object.entries and do prevent copying already allocated copies of arrays

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

After:

create spans (10 attributes) x 702,092 ops/sec ±1.32% (88 runs sampled)
create spans (10 attributes) passed as options x 603,744 ops/sec ±0.57% (97 runs sampled)
BatchSpanProcessor process span x 652,096 ops/sec ±0.97% (93 runs sampled)

Before:

create spans (10 attributes) x 691,170 ops/sec ±1.13% (94 runs sampled)
create spans (10 attributes) passed as options x 291,230 ops/sec ±0.54% (99 runs sampled)
BatchSpanProcessor process span x 645,159 ops/sec ±0.97% (95 runs sampled)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@Samuron Samuron requested a review from a team as a code owner March 20, 2024 06:27
Copy link

linux-foundation-easycla bot commented Mar 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@Samuron Samuron force-pushed the sanitize_attr_reduce_alloc branch 2 times, most recently from 5d5aca6 to 7158e6a Compare March 20, 2024 06:30
if (typeof attributes !== 'object' || attributes == null) {
return out;
}

for (const [key, val] of Object.entries(attributes)) {
for (const key in attributes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some environments (runtimes) where the usage of x in y can cause issues where the only additional fix is to also use Object.hasOwnProperty so this may have unexpected (endless looping) consequences.

It's been a while since I saw this previously, but (from memory) it is caused by a 3rd party library adding stuff to Object to support the runtime environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose to add Object.hasOwnProperty then, I think allocations is more of a problem than sheer performance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added propertyIsEnumerable check

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Merging #4558 (6acd3b5) into main (3a426e8) will increase coverage by 0.00%.
Report is 8 commits behind head on main.
The diff coverage is 88.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4558   +/-   ##
=======================================
  Coverage   92.83%   92.84%           
=======================================
  Files         328      328           
  Lines        9486     9498   +12     
  Branches     2035     2043    +8     
=======================================
+ Hits         8806     8818   +12     
  Misses        680      680           
Files Coverage Δ
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts 98.63% <100.00%> (+0.01%) ⬆️
...ckages/opentelemetry-core/src/common/attributes.ts 91.48% <85.71%> (-1.70%) ⬇️

... and 10 files with indirect coverage changes

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.

None yet

2 participants