Plotting progress metric added #2525

RomanLabGit · 2024-02-10T21:09:16Z

Code contributor checklist:

I have read, understood and followed contributing guide

nazar-pc

Thanks for your contribution!

If I'm reading this correctly, you want to be able to see total plotting progress in metrics.

While this change likely achieves that (modulo naming, it is not really sector plotting progress, it is total farm plotting progress), there is a better and more scalable way to do this.

Instead of tracking percentages, it is better to have metric about total number of sectors in the farm and number of plotted sectors. While at it, adding tracking of sectors that are about to be replotted would be nice too, that way you will be also visualize how much of the farm is still up to date and how much is being updated.

Also I would appreciate if you read contribution guidelines, but I will clean things up if necessary afterwards anyway. You really do need to clone it locally and test and not just change from the browser though.

RomanLabGit · 2024-02-11T22:46:13Z

I decided to track percentages passed into the on_sector_update callback fn to avoid using the async plotted_sectors_count fn. Is it OK to use asyncs for metrics updating? If no, could you suggest another way?

nazar-pc · 2024-02-11T22:48:37Z

You don't need to call plotted_sectors_count, you call it once during initialization and then just increment it every time new sector is plotted.

RomanLabGit · 2024-02-11T23:10:08Z

Well, this will work during initial plotting. But in case of replotting there will be an incorrect result. I assumed that the progress percentage passed via SectorPlottingDetails or the plotted_sectors_count fn provide a correct replotting result. How do you suppose to get the replotting details?

nazar-pc · 2024-02-11T23:14:06Z

Right, that was only about initial plotting. It can be done in various ways, but I'd probably make a separate metrics for replotting. When about-to-expire sectors are identified, metric will increase to that number of sectors and will decrease as sectors are replotted. And similarly for fully expired sectors.

Then you can visualize the whole plot as a combination of sectors that are:

not plotted yet
plotted and up to date
require replotting, but still viable for farming
expired

RomanLabGit · 2024-02-12T13:56:34Z

Can replotting occur during the initial plotting, before all sectors have been plotted at least once? (I hope, NO)
Can the sector update callback fn with the SectorUpdate::Expiration param be called more than once (for ex. with AboutToExpire and Expired) for the same sector before it is replotted? If that's the case, the incremental count of expired sectors you suggested (as I understand it) will not be correct.

nazar-pc · 2024-02-12T14:01:35Z

Can replotting occur during the initial plotting, before all sectors have been plotted at least once? (I hope, NO)

No, it can't (for the same farm, other farms can be replotting concurrently). It is not even known which sectors are about to expire during initial plotting, but this part will change at some point, so expired sectors will be known before initial plotting is finished (and it would be possible to show in metrics).

Can the sector update callback fn with the SectorUpdate::Expiration param be called more than once (for ex. with AboutToExpire and Expired) for the same sector before it is replotted? If that's the case, the incremental count of expired sectors you suggested (as I understand it) will not be correct.

Shouldn't be the case right now, if it ever changes we will update the code that relies on this assumption.

RomanLabGit · 2024-02-13T00:54:53Z

Please review the changes.

nazar-pc · 2024-02-13T15:54:20Z

crates/subspace-farmer/src/bin/subspace-farmer/commands/farm.rs

@@ -664,6 +664,11 @@ where

    info!("Finished collecting already plotted pieces successfully");

+    for (_, single_disk_farm) in single_disk_farms.iter().enumerate() {
+        farmer_metrics.update_farm_size(single_disk_farm.id(), single_disk_farm.total_sectors_count() as i64);


We don't want signed integers here, it should likely be SectorIndex in size (basically u16), same for below plotted sectors.

But i64 is already used in the project on the interface of the established_connections gauge. Should I use the same gauge type and convert values in the updating functions?

Yes, that is what I would prefer. We try to use domain-specific types in various places if possible and convert to the actual types of underlying libraries in case they mismatch at the boundaries.

Is it OK that single_disk_farm.plotted_sectors_count() and single_disk_farm.total_sectors_count() return different types (usize and SectorIndex)?

There are some technical reasons for it, will likely change in the future to the same type

Is it OK to use .try_into().unwrap() for converting in that case?

I would prefer as in this particular instance and we don't use .unwrap() anywhere except tests as contribution guidelines suggest.

So, should I change now?

I'll update it in the upcoming PR

nazar-pc · 2024-02-13T15:55:36Z

crates/subspace-farmer/src/bin/subspace-farmer/commands/farm.rs

+                            }
+                        }
+                        SectorUpdate::Expiration(SectorExpirationDetails::AboutToExpire) => {
+                            farmer_metrics.update_farm_expired(&single_disk_farm_id, 1);


This sector is about to expire, but it has not expired yet.

I think we cannot count the Expired and AboutToExpire sectors separately using the incremental counters only, because it isn't possible to determine which sector is replotted within the sector update callback fn. So, I decided to count them together.

We don't need to know "which" sector, we only need to know "how many" and since fully expired sectors are being replotted before those that are about to expire, we already know what to decrement.

I initially wanted to count expirations and replots cumulatively in the corresponding counter metrics and calculate the difference (actual expired sectors) outside of the farmer, this approach provides a bit more stored data. If you prefer storing only the differences, I can of course change the scheme.

I see. I guess that would work as well, but I'm not sure what would be more flexible. We already have number of sectors plotted in total with metrics before this PR. I'm open for options either way if there is some additional information to be gained from it.

nazar-pc · 2024-02-13T15:59:18Z

crates/subspace-farmer/src/bin/subspace-farmer/commands/farm.rs

+                            if let Some(_) = &old_plotted_sector {
+                                farmer_metrics.update_farm_replotted(&single_disk_farm_id, 1);
+                            } else {
+                                farmer_metrics.update_farm_plotted(&single_disk_farm_id, 1);
+                            }


All new methods are called "update", but most of them are actually increments. "update" is fine, but you then need to pass the new value as the result. Specifically, you track sectors that are about to expire and expired below to increment corresponding values, while here you decrement them, such that at the end of replotting number of expired and about to expire sectors is 0 and number of plotted sectors is the same as total number of sectors.

So most of the metrics will be gauges, not counters.

In order to track those totals you'll want to use AtomicU16 to satisfy Rust's ownership rules.

crates/subspace-farmer/src/bin/subspace-farmer/commands/farm/metrics.rs

nazar-pc · 2024-02-13T16:01:26Z

crates/subspace-farmer/src/bin/subspace-farmer/commands/farm/metrics.rs

+    pub(super) fn update_farm_size(
+        &self,
+        single_disk_farm_id: &SingleDiskFarmId,
+        sectors: i64,


Suggested change

sectors: i64,

sectors: SectorIndex,

And if you need to convert it to bigger integer prefer u64::from(sectors), this way if some of the types change in incompatible way it'll stop compiling, otherwise as u64 will potentially truncate the data. Just a good practice.

Just updated taking into account all the recommendations

nazar-pc

This is good enough, I'll do some changes on top in follow-up PR. Waiting for CI. While waiting for CI please do read contribution guidelines, we have a specific way of introducing changes that would be nice to follow next time.

nazar-pc · 2024-02-13T23:45:23Z

Well, looks like clippy is not happy, neither is formatting. https://github.com/subspace/subspace/blob/main/CONTRIBUTING.md is your friend, don't check it without actually reading.

RomanLabGit · 2024-02-14T00:42:37Z

cargo-fmt and cargo-clippy worked locally without problems now.

RomanLabGit requested review from nazar-pc, shamil-gadelshin and rg3l3dr as code owners February 10, 2024 21:09

nazar-pc reviewed Feb 10, 2024

View reviewed changes

RomanLabGit added 4 commits February 13, 2024 18:58

Update metrics.rs

419743d

Update farm.rs

5b34b12

Add files via upload

975a364

Add files via upload

8399aae

RomanLabGit force-pushed the main branch from d05df14 to 8399aae Compare February 13, 2024 15:58

nazar-pc reviewed Feb 13, 2024

View reviewed changes

Updating metrics.rs, farm.rs

aec80fd

nazar-pc previously approved these changes Feb 13, 2024

View reviewed changes

Updating metrics.rs, farm.rs

578001c

RomanLabGit dismissed nazar-pc’s stale review via 578001c February 14, 2024 00:30

nazar-pc merged commit 5e59809 into autonomys:main Feb 14, 2024
9 checks passed

nazar-pc mentioned this pull request Feb 14, 2024

Refactor farmer metrics #2531

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plotting progress metric added #2525

Plotting progress metric added #2525

RomanLabGit commented Feb 10, 2024 •

edited

Loading

nazar-pc left a comment

RomanLabGit commented Feb 11, 2024

nazar-pc commented Feb 11, 2024

RomanLabGit commented Feb 11, 2024

nazar-pc commented Feb 11, 2024

RomanLabGit commented Feb 12, 2024

nazar-pc commented Feb 12, 2024

RomanLabGit commented Feb 13, 2024

nazar-pc Feb 13, 2024

RomanLabGit Feb 13, 2024

nazar-pc Feb 13, 2024

RomanLabGit Feb 13, 2024 •

edited

Loading

nazar-pc Feb 13, 2024

RomanLabGit Feb 14, 2024

nazar-pc Feb 14, 2024

RomanLabGit Feb 14, 2024

nazar-pc Feb 14, 2024

nazar-pc Feb 13, 2024

RomanLabGit Feb 13, 2024

nazar-pc Feb 13, 2024

RomanLabGit Feb 13, 2024

nazar-pc Feb 13, 2024

nazar-pc Feb 13, 2024

nazar-pc Feb 13, 2024

RomanLabGit Feb 13, 2024

nazar-pc left a comment

nazar-pc commented Feb 13, 2024

RomanLabGit commented Feb 14, 2024

Plotting progress metric added #2525

Plotting progress metric added #2525

Conversation

RomanLabGit commented Feb 10, 2024 • edited Loading

Code contributor checklist:

nazar-pc left a comment

Choose a reason for hiding this comment

RomanLabGit commented Feb 11, 2024

nazar-pc commented Feb 11, 2024

RomanLabGit commented Feb 11, 2024

nazar-pc commented Feb 11, 2024

RomanLabGit commented Feb 12, 2024

nazar-pc commented Feb 12, 2024

RomanLabGit commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanLabGit Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc commented Feb 13, 2024

RomanLabGit commented Feb 14, 2024

RomanLabGit commented Feb 10, 2024 •

edited

Loading

RomanLabGit Feb 13, 2024 •

edited

Loading