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

Extend LATENCY LATEST to add sum / cnt stats #1570

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Jan 16, 2025

Currently LATENCY LATEST only has the following fields:

  • Event name.
  • Unix timestamp of the latest latency spike for the event.
  • Latest event latency in millisecond.
  • All-time maximum latency for this event.

This PR introduced these fields:

  • sum: the all-time sum latency for this event (i think it is useful for some events)
  • cnt: the event count that trigger the latency, with the sum we can calc the avg

Currently LATENCY LATEST only has the following fields:
- Event name.
- Unix timestamp of the latest latency spike for the event.
- Latest event latency in millisecond.
- All-time maximum latency for this event.

This PR introduced these fields:
- min: the all-time minimum latency for this event (maybe not that useful)
- sum: the all-time sum latency for this event (i think it is useful for some events)
- cnt: the event count that trigger the latency, with the sum we can calc the avg

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin requested review from a team January 16, 2025 03:20
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.02%. Comparing base (cda9eee) to head (eac30eb).
Report is 32 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1570      +/-   ##
============================================
+ Coverage     70.96%   71.02%   +0.05%     
============================================
  Files           120      121       +1     
  Lines         65094    65183      +89     
============================================
+ Hits          46191    46293     +102     
+ Misses        18903    18890      -13     
Files with missing lines Coverage Δ
src/latency.c 81.21% <100.00%> (+0.29%) ⬆️

... and 28 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think we can add this.

We are adding these in the end of an array reply. I remember we did something like that in CLUSTER SLOTS and we said it is a backward-compatible change, but still it broke some clients. For example, hiredis-cluster was checking the exact array length in the handling of CLUSTER SLOTS.

LATENCY LATEST is not as risky to change as CLUSTER SLOTS, but we should still be aware that some monitoring tools maybe can break. We should mention it in the release notes and we should add some note in the documentation that new fields can be added. A note like that makes it more clear that adding more fields in the reply is a backward compatible change. Now we don't have a note like that on https://valkey.io/commands/latency-latest/.

@zuiderkwast zuiderkwast added release-notes This issue should get a line item in the release notes major-decision-pending Major decision pending by TSC team labels Jan 21, 2025
@zuiderkwast
Copy link
Contributor

@valkey-io/core-team Please vote to extend the multi-bulk reply with three more numbers.

@hwware hwware requested a review from zuiderkwast January 27, 2025 15:51
src/latency.c Outdated Show resolved Hide resolved
Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin changed the title Extend LATENCY LATEST to add min / sum / cnt stats Extend LATENCY LATEST to add sum / cnt stats Feb 2, 2025
@enjoy-binbin enjoy-binbin added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Feb 2, 2025
@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Feb 3, 2025
@enjoy-binbin enjoy-binbin merged commit 6eaf088 into valkey-io:unstable Feb 4, 2025
50 checks passed
@enjoy-binbin enjoy-binbin deleted the latency_stats branch February 4, 2025 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Major decision approved by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants