adapt sv2-apps with channels_sv2::server::share_accounting keeping track of rejected shares

[plebhash]

companion stratum-mining/stratum#2149

draft due to 8a1422d (avoids accidental merging)

but already ready for review

[plebhash]

disclaimer: I'm not 100% confident about what's going on in this file

[plebhash]

disclaimer: I'm not 100% confident about what's going on in this file

[plebhash]

it seems this we're touching the conventions established via #469?

am I doing anything stupid?

cc @lucasbalieiro @GitGab19

[lucasbalieiro]

from what I can tell from the code, this change is expected, since we are adding two new fields to ExtendedChannelInfo and StandardChannelInfo:

• pub shares_rejected: u32
• pub shares_rejected_by_reason: HashMap<String, u32>

So the CI requesting changes here is not a false positive and in the context of the PR this change is expected. So I think you are not doing anything stupid.

If you're concerned about going against the conventions, I’d say the only guideline that might technically apply here is:

Add new data via optional fields or new endpoints - Don't modify existing response structures

That said, in this PR the change seems entirely reasonable. The guideline itself might simply be too restrictive and could probably be phrased more clearly. Something along these lines would make more sense:

Request new data through optional payload/params fields or new endpoints when appropriate. Avoid removing existing response data

adding new fields to a response is generally backward compatible because clients can safely ignore unknown fields. On the other hand, requiring new request data, especially if it is not optional, is much more likely to create compatibility issues for users.

[gimballock]

All of these changes seem to follow the correct conventions and processes, only nit i found was in the companion PR regarding use of &str instead of String to save some memory allocations.

[plebhash]

All of these changes seem to follow the correct conventions and processes, only nit i found was in the companion PR regarding use of &str instead of String to save some memory allocations.

thanks @gimballock !

so I assume this also includes prometheus_metrics.rs and snapshot_cache.rs right?

[gimballock]

All of these changes seem to follow the correct conventions and processes, only nit i found was in the companion PR regarding use of &str instead of String to save some memory allocations.

thanks @gimballock !

so I assume this also includes prometheus_metrics.rs and snapshot_cache.rs right?

You're following the conventions correctly:

• Add GaugeVec field to PrometheusMetrics
• Register with the extra label dimension
• Update in update_metrics() loop with with_label_values
• Track label sets in PreviousPrometheusLabelSets and clean stale ones

But let me deploy this now to verify the metrics actually show up in grafana..

• The rejection metric is lazy loading so i don't see it, idk how to trigger a rejection right now
• Also it could be useful to include a shares submitted metric (sum of accepted + rejected)

The lazy-loaded metrics will cause issues for alerting and make associated grafana panels fail to load until first share rejection, so that one is high priority.