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

Disable compiler optimization for noop benchmark tests #1613

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Mar 11, 2024

Changes

Gives more releastic cost for Vector of string tuple, and Vector of Key-Value.

CreateVector_KeyValue: 25.731 ns
CreateDynamicVector_StringPair: 13.419 ns

Note - should be merged after #1611

Please provide a brief description of the changes here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team as a code owner March 11, 2024 21:39
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.0%. Comparing base (440725e) to head (38f363c).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1613   +/-   ##
=====================================
  Coverage   67.0%   67.0%           
=====================================
  Files        138     138           
  Lines      19515   19515           
=====================================
  Hits       13084   13084           
  Misses      6431    6431           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cijothomas
Copy link
Member

CreateVector_KeyValue: 25.731 ns
CreateDynamicVector_StringPair: 13.419 ns

The extra ~12 ns is the OTel Tax, right?

@cijothomas cijothomas merged commit df12c2c into open-telemetry:main Mar 11, 2024
17 checks passed
@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2024

The extra ~12 ns is the OTel Tax, right?

Yes

@cijothomas
Copy link
Member

cijothomas commented Mar 13, 2024

Method AggregationTemporality Mean Error StdDev
CounterHotPath Delta 0.6230 ns 0.0052 ns 0.0043 ns
CounterWith1LabelsHotPath Delta 1.0001 ns 0.0042 ns 0.0039 ns
CounterWith2LabelsHotPath Delta 1.9753 ns 0.0276 ns 0.0258 ns
CounterWith3LabelsHotPath Delta 1.5758 ns 0.0034 ns 0.0030 ns
CounterWith4LabelsHotPath Delta 14.1514 ns 0.3069 ns 0.4201 ns

Do you think it is too much otel tax? The above benchmark results are from OTel .NET, when meterprovider is not setup. Upto 3 attributes is ~2ns only (OTel .NET internally optimized heavily for upto 3 attributes, reasonably optimized for 3-7 attributes, and not-so-optimized for 8 or more attributes)

We need to evaluate how to improve this. If opentelemetry were ever to be adopted by libraries, the no-op cost should be as close to zero. Will discuss more.

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