Skip to content

Conversation

@zasweq
Copy link
Contributor

@zasweq zasweq commented Aug 8, 2024

Contains #7484, the last commit are the cache metrics. After some discussion, decided to just do this inline with synchronous gauges with cache mutex held. OpenTelemetry just copies this data over to another store for exporters to use, so this shouldn't introduce a performance hit/lock contention, especially since the accesses cache creates lock contention on the operations even before metrics.

As suggested in #7484 (comment).

RELEASE NOTES:

  • balancer/rls: Add cache metrics

@zasweq zasweq requested a review from easwars August 8, 2024 23:44
@zasweq zasweq requested a review from dfawley August 8, 2024 23:44
@zasweq zasweq added this to the 1.66 Release milestone Aug 8, 2024
@zasweq zasweq added the Type: Feature New features or improvements in behavior label Aug 8, 2024
@easwars
Copy link
Contributor

easwars commented Aug 8, 2024

Will review this after #7484 is merged and this PR is rebased so as to only look at the changes that matter here. Thanks.

@zasweq zasweq force-pushed the add-rls-cache-metrics branch from 12f8882 to e0214a2 Compare August 9, 2024 00:08
@codecov
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 89.41176% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.46%. Comparing base (c8716e5) to head (e0214a2).
Report is 8 commits behind head on master.

Files Patch % Lines
balancer/rls/picker.go 89.36% 4 Missing and 1 partial ⚠️
internal/testutils/stats/test_metrics_recorder.go 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7494      +/-   ##
==========================================
- Coverage   81.58%   81.46%   -0.13%     
==========================================
  Files         357      357              
  Lines       27243    27315      +72     
==========================================
+ Hits        22227    22252      +25     
- Misses       3820     3844      +24     
- Partials     1196     1219      +23     
Files Coverage Δ
balancer/rls/balancer.go 85.81% <100.00%> (+0.19%) ⬆️
balancer/rls/cache.go 89.25% <100.00%> (+1.29%) ⬆️
internal/testutils/stats/test_metrics_recorder.go 69.66% <20.00%> (-2.96%) ⬇️
balancer/rls/picker.go 91.30% <89.36%> (+0.60%) ⬆️

... and 27 files with indirect coverage changes

@zasweq
Copy link
Contributor Author

zasweq commented Aug 9, 2024

Opened #7495.

@zasweq zasweq closed this Aug 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants