Skip to content

Comments

new metric in postgres_driver to estimate payload stats#3596

Merged
Ivansete-status merged 1 commit intomasterfrom
add-payload-stored-metric
Nov 24, 2025
Merged

new metric in postgres_driver to estimate payload stats#3596
Ivansete-status merged 1 commit intomasterfrom
add-payload-stored-metric

Conversation

@Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented Sep 30, 2025

Original PR: #3544

The original PR got abruptly closed after a deep cleanup and refactor applied by this on 2025-09-30

Issue

@github-actions
Copy link

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3596

Built from f3d4ca9

@Ivansete-status Ivansete-status force-pushed the add-payload-stored-metric branch from 8688ed1 to 9fe8c6f Compare November 20, 2025 06:57
@Ivansete-status Ivansete-status marked this pull request as ready for review November 20, 2025 07:41
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Need some more clarification about this.

Comment on lines +20 to +21
declarePublicGauge postgres_payload_size_bytes,
"Payload size in bytes of correctly stored messages"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't get the mean of this gauge. Is it about to show the last message payload size, or should it somehow sum up?
How should itt help in maintaining DB space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure!
The original idea is to track the average message size stored in the databases over time. This serves as a small step toward better understanding overall database usage.

)

if ret.isOk():
postgres_payload_size_bytes.set(message.payload.len)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is only set to the last message size always. Was it the aim?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless I'm missing something, it writes all messages' sizes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it writes all, just does not sum them up. That made me think the approach. But than as @fcecin gave insight I'm ok with it.

@fcecin
Copy link
Contributor

fcecin commented Nov 20, 2025

This is a "best-effort estimate" metric kept in RAM for free. The documentation provided is general and allows it to implement the estimate in different ways (sliding window/running average) in the future. There's no promise in the gauge description string that this has to be a running average of some sort, which is good. It's good to start with the simplest, N=1. I'm actually wondering how far we can go with just this.

@fcecin fcecin self-requested a review November 20, 2025 09:59
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Thank for the explanation! It is good to go!

@fcecin
Copy link
Contributor

fcecin commented Nov 20, 2025

Another thing we can do (later, or roll in this PR) is add two other gauges: total message count and total message size, then use inc() (instead of set()) on these two gauges when a message arrives (so add +1 on message count and +msg-size-bytes on the bytes counter). This way we can push whether to use a sliding window or whether to just use the simple average since start of measurement to whatever code is sitting on top of this -- the upper layer ("grafana"??) can poll the two gauges and decide to do some sliding metric, which I guess is how this is supposed work? (haven't done much metrics stuff...)

For example, the gauges at T=1: count=0 bytes=0, T=2: count=1 bytes=1000, T=3: count=2 bytes 1020. With no sliding window the simple average is 510 (1020 / 2). with a sliding-window-size=1 that excludes T=2 and only considers T=3 (dumb example, but works), the average is now 20 (20 / 1). Whoever is polling the gauges (at whatever rates) decides what to do, instead of us ever solving that problem here.

@Ivansete-status
Copy link
Collaborator Author

Thanks for the comments!
@fcecin - I think we can revisit that in the future. The gauge metric is used in values that can either increase or decrease. Grafana can extract the stats automatically for us

@Ivansete-status Ivansete-status merged commit 454b098 into master Nov 24, 2025
12 checks passed
@Ivansete-status Ivansete-status deleted the add-payload-stored-metric branch November 24, 2025 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants