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

Metric stress test with unsorted attributes. #1396

Conversation

KallDrexx
Copy link
Contributor

Changes

To better account for sorting cost, don't pass in attributes pre-sorted, as that's unlikely to be done in real applications. Likewise, add a fourth attribute for better sorting accountability.

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)

To better account for sorting cost, don't pass in attributes pre-sorted, as that's unlikely to be done in real applications.  Likewise, add a fourth attribute for better sorting accountability.
@KallDrexx KallDrexx requested a review from a team as a code owner November 22, 2023 20:18
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (073f7a6) 57.2% compared to head (ba1280e) 57.4%.
Report is 3 commits behind head on main.

Files Patch % Lines
stress/src/metrics.rs 0.0% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1396     +/-   ##
=======================================
+ Coverage   57.2%   57.4%   +0.2%     
=======================================
  Files        146     146             
  Lines      18128   18182     +54     
=======================================
+ Hits       10372   10440     +68     
+ Misses      7756    7742     -14     

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

@cijothomas
Copy link
Member

as that's unlikely to be done in real applications.

mm..Maybe we should ask users to do that to get maximum performance? It won't be enforced, so we can accept in unsorted order and do the sorting inside..
Sorting of attributes is still a perf-concern, and we need to solve it. OTel-cpp and OTel-dotnet has solved it (in diff. ways), so we should steal/copy-ideas-inspirations and do whatever it takes to fix the perf issue arising due to sorting!

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-rust/compare/main...cijothomas:perf-sort?expand=1 This benchmarks shows 18-20% perf boost, if we avoid sorting. Ofcourse it cannot be simply done, as we need to ensure correctness, but just demonstrating that sorting is a perf eater!

open-telemetry/opentelemetry-dotnet#2805 - PR showing how much OTel .NET boosted perf by avoiding sorting!

@KallDrexx
Copy link
Contributor Author

I think asking them to pre-sort attributes, and pre-dedup for best performance is fine. As far as I can tell from the Counter_Add_Unsorted and Counter_Add_Sorted, there's not a big difference for it to matter since we have to ensure they are sorted and deduplicated if they are not.

So for this PR I was mostly just trying to make sure the stress test assumes a worst case (and realistic) scenario, though I can see arguments for the existing code remaining for a "best case".

@cijothomas
Copy link
Member

Closing old PR. We dont plan to add stress tests for this particular case, but can be ad-hoc leveraged by contributors.

@cijothomas cijothomas closed this May 11, 2024
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