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

[sdk-metrics] MetricPoint lookup perf tweaks #5869

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

Conversation

CodeBlanch
Copy link
Member

Changes

  • Switch LookupAggregatorStore & LookupAggregatorStoreForDeltaWithReclaim to return a ref to the MetricPoint instead of the index to the MetricPoint. For cases where we created a MetricPoint, or reused one in the case of delta reclaim, this saves a second lookup/index/bounds-check into the array before the update is processed.

  • Use MemoryMarshal.GetArrayDataReference on TFMs where it is supported to access the zero-tag MetricPoint (index 0) and the overflow MetricPoint (index 1) instead of performing a lookup/index/bounds-check on each measurement where they are used.

  • Some renames to help with readability/clarity inside AggregatorStore...

    • LookupAggregatorStore -> LookupMetricPoint
    • LookupAggregatorStoreForDeltaWithReclaim -> LookupMetricPointForDeltaWithReclaim
    • Anything with "CustomTags" in the name now uses "WithTagFilerting" for example UpdateDoubleCustomTags -> UpdateDoubleWithTagFiltering

Benchmarks

TODO

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

@CodeBlanch CodeBlanch added the perf Performance related label Sep 27, 2024
@github-actions github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 82.51748% with 25 lines in your changes missing coverage. Please review.

Project coverage is 86.22%. Comparing base (6250307) to head (6165854).
Report is 353 commits behind head on main.

Files with missing lines Patch % Lines
src/OpenTelemetry/Metrics/AggregatorStore.cs 82.51% 25 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5869      +/-   ##
==========================================
+ Coverage   83.38%   86.22%   +2.84%     
==========================================
  Files         297      257      -40     
  Lines       12531    11220    -1311     
==========================================
- Hits        10449     9675     -774     
+ Misses       2082     1545     -537     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 86.18% <82.51%> (?)
unittests-Project-Stable 86.11% <78.32%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/OpenTelemetry/Metrics/AggregatorStore.cs 85.99% <82.51%> (+5.60%) ⬆️

... and 232 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Oct 8, 2024

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Oct 8, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf Performance related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant