Traffic-Based App Rewards: Add counters to RewardComputationTrigger#4860
Conversation
meiersi-da
left a comment
There was a problem hiding this comment.
Looks good overall. Thanks.
I'll leave the final review and approval to @rautenrieth-da
| // TODO(#4645): read num_activity_records_in_round from app_activity_round_totals | ||
| // instead of using a separate count query |
There was a problem hiding this comment.
I'm slightly uneasy about the join below, which is a potential performance snag.
Any chance to change the order of PR's and fix 4645 first?
There was a problem hiding this comment.
Or at least make sure to solve #4645 as a fast follower.
| private val prefix: MetricName = | ||
| SpliceMetrics.MetricsPrefix :+ "scan" :+ "reward_computation" | ||
|
|
||
| val activePartiesCount: Counter = metricsFactory.counter( |
There was a problem hiding this comment.
Counters are reset on each application start, so they are mostly useful for observing the rate of change in the metric (i.e., active parties added/removed per second). Is this what we are interested in?
I would have expected that we want to know what the party count was at any given time. For this, a Gauge sounds like a better metric.
There was a problem hiding this comment.
That's a good point. I originally pointed towards counters, but it seems that converting all of them to Gauge's makes more sense. They make it easy to alert on these metrics being unexpectedly high or low (e.g., no reward issued for the last hour).
Let's switch to gauges then.
@adetokunbo : note that gauges need to be closed. Please register their closing methods as it is done in other metrics classes.
ba83d25 to
cf42fda
Compare
cf42fda to
65d5745
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
65d5745 to
b34e7f8
Compare
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
a2adaea
into
adetokunbo/feature-cip-104-scan-computation
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…4860) Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Merges the feature branch for the scan computation part of CIP-104, consisting of the following individual PRs: * Add app activity computation store and schema (#4372) * Compute aggregateActivityTotals, add ComputeAppRewardTrigger, expose new endpoints (#4420) * Adds computeRewardTotals to DbScanAppRewardsStore (#4639) * Add DB versions of Splice.Amulet.CryptoHash (#4662) * Expand GetRewardAccountingActivityTotalsResponse (#4859) * Add counters to RewardComputationTrigger (#4860) * Compute Hashes (#4847) Signed-off-by: Tim Emiola <adetokunbo@emio.la> Signed-off-by: Timothy Emiola <adetokunbo@users.noreply.github.com> Co-authored-by: Simon Meier <simon@digitalasset.com> Co-authored-by: Robert Autenrieth <31539813+rautenrieth-da@users.noreply.github.com>
Fixes #4634
Follow-up to this comment in #4420
Stacked on
#4847#4859 that's currently being reviewedTracked in #4384
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines