kvs: add transaction stats #6556

chu11 · 2025-01-16T22:48:21Z

Problem: In the future we would like to put a cap on the maximum number
of operations in a transaction. However, we do not have any stat tracking
of operations in a transaction.

Add transaction stats for commits and fences in the kvs module.

This is really a "prep" for #6125. I decided to split out the stats for commits & fences, since they are quite different. For example, if every process in an job did a fenced KVS transaction with a single op, this scales up with the size of cluster. So we want to count that differently than each op individually.

Note that I could have done "stats per namespace", but that wasn't really the information that we want. We mostly want to know what is the average and what is the max. So I did it "globally" vs per namespace.

chu11 · 2025-01-17T07:04:30Z

oops, looks like our CI doesn't have bc so everything failed :-) Re-worked the tests to use jq instead (actually is much cleaner now).

Edit: agh ... try again. CI didn't like my use of single quotes.

chu11 · 2025-01-18T00:34:56Z

oops, i totally forgot about stats clearing. Tweaked and added a test.

garlick

LGTM once the test failures are worked out.

I did just have a couple minor suggestions.

Also a passing comment that we should fix the stats key names that have spaces and # in them at some point.

garlick · 2025-01-21T15:42:00Z

src/modules/kvs/kvs.c

+    if (!(txncstats = json_pack ("{ s:i s:i s:f s:f s:i }",
+                                 "count", tstat_count (&ctx->txn_commit_stats),
+                                 "min", (int)tstat_min (&ctx->txn_commit_stats),
+                                 "mean", tstat_mean (&ctx->txn_commit_stats),
+                                 "stddev", tstat_stddev (&ctx->txn_commit_stats),
+                                 "max",
+                                   (int)tstat_max (&ctx->txn_commit_stats))))
+        goto nomem;
+
+    if (!(txnfstats = json_pack ("{ s:i s:i s:f s:f s:i }",
+                                 "count", tstat_count (&ctx->txn_fence_stats),
+                                 "min", (int)tstat_min (&ctx->txn_fence_stats),
+                                 "mean", tstat_mean (&ctx->txn_fence_stats),
+                                 "stddev", tstat_stddev (&ctx->txn_fence_stats),
+                                 "max", (int)tstat_max (&ctx->txn_fence_stats))))
+        goto nomem;


This is the same as the tstats object except max is packed as an intetger.

Would adding a add_tstat_to_object() function be useful here to get all three in the same format, reduce code duplication, and avoid the need for a temporary variable per object?

Sounds like a good idea. However, I do dislike that the min and max for op-count would be a double if we created a common function for this. Although it's not the end of the world if that were the case.

For the cache stats, I suppose min and max should also always be integers (or big Ints) too. However, that would be changing the output format. Hmmm, lemme ponder this detail for a bit.

Changing the output format here doesn't seem so bad.

garlick · 2025-01-21T15:50:41Z

src/modules/kvs/kvs.c

                           "cache", cstats,
                           "namespace", nsstats,
+                           "transactions",


Suggestion: make this key more descriptive like transaction-opcount?

Problem: The min/max cache object size stat is returned as floating point, even though such a statistic cannot have a decimal place by definition (in contrast to lets say "mean" or "stdev"). Return the min/max cache object size as an integer instead of a floating point.

Problem: In the future we would like to put a cap on the maximum number of operations in a transaction. However, we do not have any stat tracking of operations in a transaction. Add transaction stats for commits and fences in the kvs module.

Problem: The testing of stats clearing in the KVS is done a little bit before the end of a set of tests. This can be inconvenient for future tests that do not wish stats to be cleared. Move the clearing of stats tests towards the end of tests in t1001-kvs-internals.t.

Problem: There is no coverage for the new KVS transaction stats. Add coverage in t1001-kvs-internals.t.

chu11 · 2025-01-21T22:05:12Z

ok, re-pushed per comments above. The big change is that the cache object stats now use ints for the output of min and max.

That allowed me to create a convenience function for the cache object stats & the new transaction stats to generation objects for tstat.

I elected to go with the convenience function instead of a new tstats function b/c of A) the casting of the min/max to ints and B) the scaling option to scale the stats (used by cache object stats). It just seemed like too much special stuff just for the kvs.

garlick · 2025-01-21T22:30:43Z

src/modules/kvs/kvs.c

    json_decref (tstats);
    json_decref (cstats);
+    json_decref (txncstats);
+    json_decref (txnfstats);
    json_decref (nsstats);


If the convenience function added a key to an existing object rather than return an object, it would save on the temp variables.

I had thought about that, but that's only for the first case w/ the object stats here.

if (!(cstats = json_pack ("{ s:f s:O s:i s:i s:i }", "obj size total (MiB)", (double)size/1048576, "obj size (KiB)", tstats, "#obj dirty", dirty, "#obj incomplete", incomplete, "#faults", ctx->faults))) goto nomem;

Most of the "object' construction is done with a rpc response at the end:

if (flux_respond_pack (h, msg, "{ s:O s:O s:{s:O s:O} s:i }", "cache", cstats, "namespace", nsstats, "transaction-opcount", "commit", txncstats, "fence", txnfstats, "pending_requests", zhashx_size (ctx->requests)) < 0) flux_log_error (h, "%s: flux_respond_pack", __FUNCTION__);

we could create a "transaction-ops" object with json_object() and pass it around. It seemed like a "meh" win to me ... feel strongly about it?

chu11 · 2025-01-21T23:14:12Z

thanks, setting MWP

codecov · 2025-01-21T23:15:41Z

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.46%. Comparing base (d32d543) to head (dffe9e4).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/modules/kvs/kvs.c	78.57%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #6556       +/-   ##
===========================================
+ Coverage   41.41%   79.46%   +38.04%     
===========================================
  Files         478      531       +53     
  Lines       79878    88309     +8431     
===========================================
+ Hits        33082    70171    +37089     
+ Misses      46796    18138    -28658

Files with missing lines	Coverage Δ
src/modules/kvs/kvs.c	`72.43% <78.57%> (+31.91%)`	⬆️

... and 475 files with indirect coverage changes

chu11 force-pushed the kvs_txn_stats branch from d35f515 to 1789bd2 Compare January 17, 2025 07:03

chu11 force-pushed the kvs_txn_stats branch 5 times, most recently from 4506ffa to d8deacd Compare January 18, 2025 00:34

garlick approved these changes Jan 21, 2025

View reviewed changes

chu11 added 4 commits January 21, 2025 13:27

kvs: add transaction stats

af7a42b

Problem: In the future we would like to put a cap on the maximum number of operations in a transaction. However, we do not have any stat tracking of operations in a transaction. Add transaction stats for commits and fences in the kvs module.

t: cover kvs transaction stats

dffe9e4

Problem: There is no coverage for the new KVS transaction stats. Add coverage in t1001-kvs-internals.t.

chu11 force-pushed the kvs_txn_stats branch from d8deacd to dffe9e4 Compare January 21, 2025 22:02

garlick reviewed Jan 21, 2025

View reviewed changes

chu11 added the merge-when-passing label Jan 21, 2025

mergify bot merged commit 2088011 into flux-framework:master Jan 21, 2025
35 checks passed

chu11 deleted the kvs_txn_stats branch January 21, 2025 23:57

chu11 mentioned this pull request Jan 25, 2025

WIP: kvs: support configuration of max transaction count #6581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvs: add transaction stats #6556

kvs: add transaction stats #6556

chu11 commented Jan 16, 2025

chu11 commented Jan 17, 2025 •

edited

Loading

chu11 commented Jan 18, 2025

garlick left a comment

garlick Jan 21, 2025

chu11 Jan 21, 2025

garlick Jan 21, 2025

garlick Jan 21, 2025

chu11 commented Jan 21, 2025

garlick Jan 21, 2025 •

edited

Loading

chu11 Jan 21, 2025

garlick Jan 21, 2025

chu11 commented Jan 21, 2025

codecov bot commented Jan 21, 2025

kvs: add transaction stats #6556

kvs: add transaction stats #6556

Conversation

chu11 commented Jan 16, 2025

chu11 commented Jan 17, 2025 • edited Loading

chu11 commented Jan 18, 2025

garlick left a comment

Choose a reason for hiding this comment

garlick Jan 21, 2025

Choose a reason for hiding this comment

chu11 Jan 21, 2025

Choose a reason for hiding this comment

garlick Jan 21, 2025

Choose a reason for hiding this comment

garlick Jan 21, 2025

Choose a reason for hiding this comment

chu11 commented Jan 21, 2025

garlick Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

chu11 Jan 21, 2025

Choose a reason for hiding this comment

garlick Jan 21, 2025

Choose a reason for hiding this comment

chu11 commented Jan 21, 2025

codecov bot commented Jan 21, 2025

Codecov Report

chu11 commented Jan 17, 2025 •

edited

Loading

garlick Jan 21, 2025 •

edited

Loading