substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Move Throughput into `sc-sysinfo`

Open Szegoo opened this issue 3 years ago • 8 comments

Prepares for #12017

Szegoo avatar Sep 27 '22 15:09 Szegoo

I think it is also fine to split the issue into two MRs, depending on your preferences.
The changes here already make sense to have on their own.

ggwpez avatar Sep 28 '22 20:09 ggwpez

I think it is also fine to split the issue into two MRs, depending on your preferences. The changes here already make sense to have on their own.

Sure, that makes sense.

Szegoo avatar Sep 28 '22 20:09 Szegoo

I can review earliest on Friday. But maybe @koute wants to, it touches your code :)

You are only reafactoring code as preparation for #12017, right?
Then please update the issue description to not Close the issue.
We also need to ensure that this does not break telemetry.

ggwpez avatar Sep 28 '22 20:09 ggwpez

You are only reafactoring code as preparation for #12017, right? Then please update the issue description to not Close the issue. We also need to ensure that this does not break telemetry.

Yes, this is only preparation for #12017, I changed the description now. The CI doesn't seem happy, I will do some minor fixes on this.

Szegoo avatar Sep 28 '22 20:09 Szegoo

I also don't really get why we need Throughput there. @ggwpez can you give more context?

bkchr avatar Sep 28 '22 21:09 bkchr

I also don't really get why we need Throughput there. @ggwpez can you give more context?

The measurement of the performance at startup runs in sc-sysinfo. So if we now want to compare the results of that to anything, we need the Throughput types to have nicer compare logic and formatting in the final output. The type only exists to make it possible to deserialize from JSON with a unit suffix.
I just originally did not put it in sy-sysinfo to keep the sc- crate small, since it was not needed.

ggwpez avatar Sep 29 '22 14:09 ggwpez

Should I move Metric also? Or should that be done in a separate PR?

Szegoo avatar Oct 02 '22 13:10 Szegoo

Should I move Metric also? Or should that be done in a separate PR?

Let's do that in a separate PR.

koute avatar Oct 03 '22 06:10 koute

@ggwpez So what is the final decision then?

Szegoo avatar Oct 20 '22 15:10 Szegoo

@ggwpez So what is the final decision then?

Okay then lets remove the unit from the serialization. Less code - less bugs.

ggwpez avatar Oct 20 '22 17:10 ggwpez

Okay then lets remove the unit from the serialization. Less code - less bugs.

@ggwpez What should we do with reference_hardware.json then?

Edit: wouldn't auto-generating it result in even more code?

Szegoo avatar Oct 20 '22 17:10 Szegoo

@ggwpez What should we do with reference_hardware.json then?

Change it so that it works with the new format.

Edit: wouldn't auto-generating it result in even more code?

We need auto-generation at one point anyway, as manual editing is error-prone and not straight forward.

ggwpez avatar Oct 29 '22 08:10 ggwpez

@ggwpez So I assume we should use MiBs in the reference_hardware.json then?

Szegoo avatar Oct 29 '22 12:10 Szegoo

@ggwpez can we merge this?

bkchr avatar Nov 03 '22 22:11 bkchr

bot merge

ggwpez avatar Nov 04 '22 16:11 ggwpez

Waiting for commit status.