Report vstorage size delta for set() operations via telemetry #10997

siarhei-agoric · 2025-02-12T21:43:48Z

Description

add vstorage_size_delta metric to vstorage keeper. The metric is reported for each Set() and Delete() storage operation.

Security Considerations

none

Scaling Considerations

This change adds an explicit Get() call for each Set() / Delete() operation

Documentation Considerations

cosmos telemetry will emit a new metric vstorage_size_delta

Testing Considerations

Manual testing.

First terminal:

cd packages/cosmic-swingset
make
make scenario2-setup
make scenario2-run-chain-economy

Second terminal:

while true; do curl -s -G 'http://localhost:26660/metrics' | grep store_size_delta; sleep 10; done;

Output after a long startup delay:

# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 61594
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 61769
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 61769
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 63002
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 71443
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 71443
# HELP store_size_delta store_size_delta
# TYPE store_size_delta counter
store_size_delta{storeKey="vstorage"} 71443

Upgrade Considerations

none

golang/cosmos/x/vstorage/vstorage.go

siarhei-agoric · 2025-02-12T21:50:25Z

golang/cosmos/x/vstorage/vstorage.go

@@ -101,6 +147,7 @@ func (sh vstorageHandler) Receive(cctx context.Context, str string) (ret string,
 			if err != nil {
 				return
 			}
+			reportKVEntrySizeDelta(ctx, keeper, entry, msg.Method)


do we need to add a similar call to reportKVEntrySizeDelta() in case "append": below?
if yes, what should it be reporting as it is not clear to me how exactly "append" works.

We definitely need this for "append". The implementation is in vstorage/keeper/keeper.go, and the summary is that inbound data extends rather than replaces preëxisting data except at block boundaries (for which the old data is permanently committed into history and can be recovered by querying for the previous block). And as @mhofman suggests, we really want the delta calulation at that point anyway.

mhofman

I think we need to do the calculation of size delta closer to the DB access. Instrumenting at Receive causes us to perform extra DB operations which are expensive. It also cannot properly account for the "update" an existing entry case, especially in the "append" mode. The keeper already needs to read the DB, so it should be a much better place to do the accounting.

refs: #10938

cloudflare-workers-and-pages · 2025-02-18T16:28:19Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`a8e9c91`
Status:	✅ Deploy successful!
Preview URL:	https://d4edc90a.agoric-sdk.pages.dev
Branch Preview URL:	https://sliakh-10938-vstorage-write.agoric-sdk.pages.dev

View logs

gibson042

The overall modifications seem good to me, but looking at the telemetry package source code, we should anticipate a future in which we account for all chain storage and label metrics by storeKey. I've added corresponding suggestions.

Testing coverage would be nice, but those gaps are neither new nor exacerbated by this work. But can you document the process and results of manual testing in this PR?

golang/cosmos/x/vstorage/keeper/keeper.go

Co-authored-by: Richard Gibson <[email protected]>

refs: #10938

golang/cosmos/x/vstorage/keeper/keeper.go

refs: #10938

cloudflare-workers-and-pages · 2025-02-20T23:05:34Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`36bdd97`
Status:	✅ Deploy successful!
Preview URL:	https://73068153.agoric-sdk.pages.dev
Branch Preview URL:	https://sliakh-10938-vstorage-write.agoric-sdk.pages.dev

View logs

cloudflare-workers-and-pages · 2025-02-21T01:58:13Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1596289`
Status:	⚡️ Build in progress...

View logs

cloudflare-workers-and-pages · 2025-02-21T02:02:43Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1596289`
Status:	✅ Deploy successful!
Preview URL:	https://f262b963.agoric-sdk.pages.dev
Branch Preview URL:	https://sliakh-10938-vstorage-write.agoric-sdk.pages.dev

View logs

cloudflare-workers-and-pages · 2025-02-21T14:46:27Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`b5e1d42`
Status:	⚡️ Build in progress...

View logs

cloudflare-workers-and-pages · 2025-02-21T14:51:13Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`b5e1d42`
Status:	✅ Deploy successful!
Preview URL:	https://fcd11148.agoric-sdk.pages.dev
Branch Preview URL:	https://sliakh-10938-vstorage-write.agoric-sdk.pages.dev

View logs

gibson042 · 2025-02-21T14:56:41Z

can you document the process and results of manual testing in this PR?

@siarhei-agoric where did we land on this?

siarhei-agoric · 2025-02-21T16:53:00Z

can you document the process and results of manual testing in this PR?

@siarhei-agoric where did we land on this?

Sorry, missed that first time around. Updated the "Testing Considerations" section in OP.

mhofman

Doing this at the level of the keeper is so much cleaner, thanks for this!

It looks like cosmos-sdk always reports these telemetry in a defer, I'm wondering why.

golang/cosmos/x/vstorage/keeper/keeper.go

mhofman · 2025-02-21T20:35:35Z

Removed automerge label to let you address question. Feel free to reapply if no changes needed.

refs: #10938

gibson042 · 2025-02-21T23:31:23Z

It looks like cosmos-sdk always reports these telemetry in a defer, I'm wondering why.

My guess is to ensure that errors from attempting to update telemetry don't interfere with the business logic, although that seems of dubious value to me.

siarhei-agoric requested review from mhofman and gibson042 February 12, 2025 21:44

siarhei-agoric commented Feb 12, 2025

View reviewed changes

golang/cosmos/x/vstorage/vstorage.go Outdated Show resolved Hide resolved

siarhei-agoric commented Feb 12, 2025

View reviewed changes

mhofman requested changes Feb 14, 2025

View reviewed changes

siarhei-agoric added 2 commits February 18, 2025 11:28

feat(cosmos:vstorage): report vstorage size delta for set() operations

af42bd8

refs: #10938

feat(cosmos/vstorage): move telemetry onto keeper.go

81840a4

refs: #10938

siarhei-agoric force-pushed the sliakh-10938-vstorage-write-metrics branch from 916a907 to 81840a4 Compare February 18, 2025 16:28

siarhei-agoric requested a review from mhofman February 18, 2025 16:33

siarhei-agoric marked this pull request as ready for review February 18, 2025 16:35

siarhei-agoric requested a review from a team as a code owner February 18, 2025 16:35

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

32114f6

siarhei-agoric added the automerge:squash Automatically squash merge label Feb 18, 2025

siarhei-agoric added 3 commits February 18, 2025 18:33

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

7be8792

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

4f1a176

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

3c2ad9c

siarhei-agoric removed the automerge:squash Automatically squash merge label Feb 19, 2025

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

8808ba3

siarhei-agoric added the automerge:squash Automatically squash merge label Feb 19, 2025

gibson042 requested changes Feb 19, 2025

View reviewed changes

fixup: Apply suggestions from code review

95eb821

Co-authored-by: Richard Gibson <[email protected]>

siarhei-agoric removed the automerge:squash Automatically squash merge label Feb 19, 2025

fixup(cosmos/x/vstorage): add labels to metrics in keeper

151c545

refs: #10938

siarhei-agoric requested a review from gibson042 February 19, 2025 19:28

siarhei-agoric added 2 commits February 19, 2025 14:28

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

2c3705a

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

3176dc8

gibson042 requested changes Feb 20, 2025

View reviewed changes

golang/cosmos/x/vstorage/keeper/keeper.go Outdated Show resolved Hide resolved

fixup(cosmos/vstorage): Incorporate PR review comments

43e8481

refs: #10938

gibson042 approved these changes Feb 20, 2025

View reviewed changes

siarhei-agoric added the automerge:squash Automatically squash merge label Feb 20, 2025

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

36bdd97

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

1596289

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

b5e1d42

siarhei-agoric removed the automerge:squash Automatically squash merge label Feb 21, 2025

siarhei-agoric added the automerge:squash Automatically squash merge label Feb 21, 2025

mhofman approved these changes Feb 21, 2025

View reviewed changes

golang/cosmos/x/vstorage/keeper/keeper.go Outdated Show resolved Hide resolved

golang/cosmos/x/vstorage/keeper/keeper.go Outdated Show resolved Hide resolved

mhofman removed the automerge:squash Automatically squash merge label Feb 21, 2025

fixup(cosmos/x/vstorage): incorporate review feedback

e07f68f

refs: #10938

siarhei-agoric requested a review from mhofman February 21, 2025 22:47

siarhei-agoric added the automerge:squash Automatically squash merge label Feb 21, 2025

Merge branch 'master' into sliakh-10938-vstorage-write-metrics

a8e9c91

mergify bot merged commit 252b405 into master Feb 21, 2025
84 checks passed

mergify bot deleted the sliakh-10938-vstorage-write-metrics branch February 21, 2025 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report vstorage size delta for set() operations via telemetry #10997

Report vstorage size delta for set() operations via telemetry #10997

siarhei-agoric commented Feb 12, 2025 •

edited

Loading

siarhei-agoric Feb 12, 2025

gibson042 Feb 14, 2025

mhofman left a comment

cloudflare-workers-and-pages bot commented Feb 18, 2025 •

edited

Loading

gibson042 left a comment

cloudflare-workers-and-pages bot commented Feb 20, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

gibson042 commented Feb 21, 2025

siarhei-agoric commented Feb 21, 2025

mhofman left a comment

mhofman commented Feb 21, 2025

gibson042 commented Feb 21, 2025

Report vstorage size delta for set() operations via telemetry #10997

Report vstorage size delta for set() operations via telemetry #10997

Conversation

siarhei-agoric commented Feb 12, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

siarhei-agoric Feb 12, 2025

Choose a reason for hiding this comment

gibson042 Feb 14, 2025

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 18, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

gibson042 left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Feb 20, 2025

Deploying agoric-sdk with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

gibson042 commented Feb 21, 2025

siarhei-agoric commented Feb 21, 2025

mhofman left a comment

Choose a reason for hiding this comment

mhofman commented Feb 21, 2025

gibson042 commented Feb 21, 2025

siarhei-agoric commented Feb 12, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 18, 2025 •

edited

Loading