Skip to content

Conversation

@jasuwienas
Copy link
Contributor

@jasuwienas jasuwienas commented Dec 5, 2025

Description

The logic responsible for collecting metrics is being moved into a separate service.
The CacheService is being updated to implement the ClientInterface (which effectively makes the CacheService itself redundant). It will be kept only as an alias for the client.

Related issue(s)

Fixes #4690

Testing Guide

  1. Run tests
  2. Check if they pass

Changes from original design (optional)

N/A

Additional work needed (optional)

Deprecatd methods can be removed, here is the implementation: jasuwienas#2. (let me know wdyt)

Checklist

  • I've assigned an assignee to this PR and related issue(s) (if applicable)
  • I've assigned a label to this PR and related issue(s) (if applicable)
  • I've assigned a milestone to this PR and related issue(s) (if applicable)
  • I've updated documentation (code comments, README, etc. if applicable)
  • I've done sufficient testing (unit, integration, etc.)

@jasuwienas jasuwienas self-assigned this Dec 5, 2025
@jasuwienas jasuwienas added the enhancement New feature or request label Dec 5, 2025
@jasuwienas jasuwienas added this to the 0.74.0 milestone Dec 5, 2025
@jasuwienas jasuwienas linked an issue Dec 5, 2025 that may be closed by this pull request
@jasuwienas jasuwienas force-pushed the 4558-cache-measurements-by-decorator branch from 48abcac to f1f1ad0 Compare December 5, 2025 16:50
@jasuwienas jasuwienas linked an issue Dec 5, 2025 that may be closed by this pull request
@jasuwienas
Copy link
Contributor Author

Important note for reviewers!
After previous consultations, I decided to introduce two additional changes, which may be breaking:

  1. Timing of Prometheus communication

Previously, Prometheus updates occurred sometimes before and sometimes after Redis calls, depending on the method. This inconsistency has now been removed. However, it’s unclear whether the original behavior was intentional, so please keep an eye out for any implications.

  1. Removal of the client-specific Prometheus counter mapping

In the last commit, I removed the map that translated certain Prometheus methods calls counters depending on the client used.

For example, calling incrBy using the Redis client used to increment the Prometheus incrBy metric, but calling incrBy through the internal client counted as a GET or SET.

Removing this mapping makes the code cleaner and easier to read, and I don’t believe we lose any critical information (e.g., LRU reads can still be calculated by summing get and incrBy calls).

However, if any existing events or logic rely on the old mapping behavior, they may break. If you are aware of anything that depends on this mapping, let me know.

@jasuwienas jasuwienas marked this pull request as ready for review December 5, 2025 17:18
@jasuwienas jasuwienas requested review from a team as code owners December 5, 2025 17:18
@jasuwienas jasuwienas requested a review from acuarica December 5, 2025 17:18
@jasuwienas jasuwienas force-pushed the 4558-cache-measurements-by-decorator branch from a23dd2d to 8336fbe Compare December 5, 2025 17:32
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 87.14859% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ges/relay/src/lib/clients/cache/measurableCache.ts 90.22% 17 Missing ⚠️
...ckages/relay/src/lib/clients/cache/ICacheClient.ts 0.00% 5 Missing ⚠️
...elay/src/lib/services/cacheService/cacheService.ts 0.00% 3 Missing ⚠️
...kages/relay/src/lib/clients/cache/localLRUCache.ts 84.61% 2 Missing ⚠️
...lay/src/lib/clients/cache/redisCache/redisCache.ts 84.61% 2 Missing ⚠️
...src/lib/clients/cache/redisCache/safeRedisCache.ts 84.61% 2 Missing ⚠️
...ages/relay/src/lib/factories/cacheClientFactory.ts 95.65% 1 Missing ⚠️

❌ Your project check has failed because the head coverage (68.80%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (75f8196) and HEAD (8336fbe). Click for more details.

HEAD has 22 uploads less than BASE
Flag BASE (75f8196) HEAD (8336fbe)
19 1
config-service 1 0
relay 1 0
server 1 0
ws-server 1 0
@@             Coverage Diff             @@
##             main    #4688       +/-   ##
===========================================
- Coverage   95.67%   68.80%   -26.88%     
===========================================
  Files         131      132        +1     
  Lines       21028    21030        +2     
  Branches     1785      538     -1247     
===========================================
- Hits        20118    14469     -5649     
- Misses        892     6544     +5652     
+ Partials       18       17        -1     
Flag Coverage Δ
config-service ?
relay ?
server ?
ws-server ?

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

Files with missing lines Coverage Δ
packages/relay/src/lib/clients/index.ts 100.00% <100.00%> (ø)
packages/relay/src/lib/relay.ts 95.45% <100.00%> (-3.20%) ⬇️
...ages/relay/src/lib/factories/cacheClientFactory.ts 97.56% <95.65%> (-2.44%) ⬇️
...kages/relay/src/lib/clients/cache/localLRUCache.ts 58.90% <84.61%> (-38.54%) ⬇️
...lay/src/lib/clients/cache/redisCache/redisCache.ts 75.16% <84.61%> (-23.78%) ⬇️
...src/lib/clients/cache/redisCache/safeRedisCache.ts 93.51% <84.61%> (-6.49%) ⬇️
...elay/src/lib/services/cacheService/cacheService.ts 0.00% <0.00%> (-100.00%) ⬇️
...ckages/relay/src/lib/clients/cache/ICacheClient.ts 0.00% <0.00%> (ø)
...ges/relay/src/lib/clients/cache/measurableCache.ts 90.22% <90.22%> (ø)

... and 71 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move cache measurement logic into separate service Create factory for CacheClient

1 participant