-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(congestion): write stats to a csv file #10719
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10719 +/- ##
==========================================
- Coverage 71.72% 71.66% -0.07%
==========================================
Files 758 758
Lines 152593 152680 +87
Branches 152593 152680 +87
==========================================
- Hits 109451 109418 -33
- Misses 38191 38295 +104
- Partials 4951 4967 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
<p>This PR was automatically created by Snyk using the credentials of a real user.</p><br /><h3>Snyk has created this PR to upgrade react-router-dom from 6.19.0 to 6.20.0.</h3> :information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project. <hr/> - The recommended version is **2 versions** ahead of your current version. - The recommended version was released **21 days ago**, on 2023-11-22. <details> <summary><b>Release notes</b></summary> <br/> <details> <summary>Package name: <b>react-router-dom</b></summary> <ul> <li> <b>6.20.0</b> - <a href="https://snyk.io/redirect/github/remix-run/react-router/releases/tag/react-router-native%406.20.0">2023-11-22</a></br><p>react-router-native@6.20.0</p> </li> <li> <b>6.20.0-pre.0</b> - 2023-11-21 </li> <li> <b>6.19.0</b> - 2023-11-16 </li> </ul> from <a href="https://snyk.io/redirect/github/remix-run/react-router/releases">react-router-dom GitHub release notes</a> </details> </details> <details> <summary><b>Commit messages</b></summary> </br> <details> <summary>Package name: <b>react-router-dom</b></summary> <ul> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/3cc38eac4753702a9a8a1fe239e2138d63ac6cc5">3cc38ea</a> chore: Update version for release (near#11050)</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/77862f95f71397d92b8b583a16674c56efccc55f">77862f9</a> Exit prerelease mode</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/1621319ffa353bf33f33064d7611859df16286ee">1621319</a> Update Release Notes TOC</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/f1f8ed0acffb3d6c2860c362fc2b376dbf87df24">f1f8ed0</a> Update release notes</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/1e026b6f1ac34a774b4f77e5e3696251e8f79940">1e026b6</a> chore: Update version for release (pre) (near#11047)</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/4a08d64c368c07816e753632345a13b8da050111">4a08d64</a> Enter prerelease mode</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/c0b4e12d12c5abdf5f7723e71959c4eb5e9effd9">c0b4e12</a> Merge branch 'main' into release-next</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/58d421fc4c592661a68dea59edc507fc4668ba5d">58d421f</a> Fix other code paths for resolveTo from a splat route (near#11045)</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/5f530a775cd266940f725894277b6ea7bc55b5d0">5f530a7</a> Do not revalidate unmounted fetchers when v7_persistFetcher is enabled (near#11044)</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/f320378b5145f59bb266a35a7655b563f712daef">f320378</a> Add additional test case for near#10983</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/a48c43c8118bbf75b23b3ee748648bb3ee4d688e">a48c43c</a> feat: export `PathParam` type (near#10719)</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/1d56e55d3f95730f99617dff23cf153f82394921">1d56e55</a> Remove tag links from headings in CHANGELOG.md</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/3b1a7364c730209b4baed9454c7f6c17c55e3ba8">3b1a736</a> Fix flaky test</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/406a1ddecd399ede2a517d7cae5f3ee63d02ed91">406a1dd</a> Merge branch 'release-next' into dev</li> <li><a href="https://snyk.io/redirect/github/remix-run/react-router/commit/6a5939b07c06c9dacd82705645dbbbe46de90e5e">6a5939b</a> Merge branch 'release-next'</li> </ul> <a href="https://snyk.io/redirect/github/remix-run/react-router/compare/dcf0c2a85aac3a78059a287ea478ff12adcb6a2d...3cc38eac4753702a9a8a1fe239e2138d63ac6cc5">Compare</a> </details> </details> <hr/> **Note:** *You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.* For more information: <img src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiIyZmI3YjkxYi0zN2NiLTQzYzgtOWNkYS02ODAzNTFmYzMwNmQiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjJmYjdiOTFiLTM3Y2ItNDNjOC05Y2RhLTY4MDM1MWZjMzA2ZCJ9fQ==" width="0" height="0"/> 🧐 [View latest project report](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763?utm_source=github&utm_medium=referral&page=upgrade-pr) 🛠 [Adjust upgrade PR settings](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763/settings/integration?utm_source=github&utm_medium=referral&page=upgrade-pr) 🔕 [Ignore this dependency or unsubscribe from future upgrade PRs](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763/settings/integration?pkg=react-router-dom&utm_source=github&utm_medium=referral&page=upgrade-pr#auto-dep-upgrades) <!--- (snyk:metadata:{"prId":"2fb7b91b-37cb-43c8-9cda-680351fc306d","prPublicId":"2fb7b91b-37cb-43c8-9cda-680351fc306d","dependencies":[{"name":"react-router-dom","from":"6.19.0","to":"6.20.0"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"98480bdc-d80b-4fd1-89d7-c4c56a706763","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":2,"publishedDate":"2023-11-22T16:56:32.720Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]}) ---> Co-authored-by: snyk-bot <snyk-bot@snyk.io> Co-authored-by: nikurt <86772482+nikurt@users.noreply.github.com>
0de8198
to
e2503f2
Compare
A few more stats need to be added as well as stats for other strategies but I'll leave that for later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this conceptually! Getting all the data out in a CSV is exactly what we need to solve the evaluation part cleanly. The summary table was a very crude first example of how we could get an output, this is miles better.
However, you are breaking several of the code architecture invariants. 😅 Not your fault, of course, you couldn't know it. But the way I had it in my mind, I wanted to keep the code base modular with minimal cross-over between the 4 main components (strategy, workload, model, evaluation). You can kind of see it how your changes forced you to pass around the stats writer to many files that shouldn't be affected by such a change.
I've tried to explain what my architectural idea is in in-line comments. Please take a look and let me know if you agree / want to follow my proposed architecture. Or if you think it's too much work or just not a good idea, we can can also work out a different architecture together, I'm open to your suggestions.
e2503f2
to
30ccf14
Compare
@@ -23,17 +23,17 @@ impl QueueBundle { | |||
}; | |||
|
|||
for &shard in shards { | |||
let mailbox = this.new_queue(shard); | |||
let mailbox = this.new_queue(shard, "mailbox"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the heck is mailbox? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borrowed vocabulary from the actor model
https://www.brianstorti.com/the-actor-model/
The model basically treats each shard as an actor and the implicit incoming receipts queue is the actor's mailbox. And that's no coincidence. I always like to map problems to solutions that already exist.
Not only is the model implemented this way, I think even the real implementation can be thought of this way. Taking inspiration from Erlang, Akka, Actix, and other established actor frameworks could be useful. But it's also quite limited since they generally react to full queues by dropping messages / returning failures (which I still think we can avoid) and they don't operate on a global round-based clock that the blockchain has thanks to block heights. I think those are the main differences in constraints.
Anyway, sorry for the off-topic rambling... "mailbox" as the queue name makes sense in my mind but open to other names :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wacban for the changes! I personally like this much better, as it now fits in perfectly with the intended architecture.
This is going to be suuper helpful to analyse and compare different strategies!
@@ -23,17 +23,17 @@ impl QueueBundle { | |||
}; | |||
|
|||
for &shard in shards { | |||
let mailbox = this.new_queue(shard); | |||
let mailbox = this.new_queue(shard, "mailbox"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borrowed vocabulary from the actor model
https://www.brianstorti.com/the-actor-model/
The model basically treats each shard as an actor and the implicit incoming receipts queue is the actor's mailbox. And that's no coincidence. I always like to map problems to solutions that already exist.
Not only is the model implemented this way, I think even the real implementation can be thought of this way. Taking inspiration from Erlang, Akka, Actix, and other established actor frameworks could be useful. But it's also quite limited since they generally react to full queues by dropping messages / returning failures (which I still think we can avoid) and they don't operate on a global round-based clock that the blockchain has thanks to block heights. I think those are the main differences in constraints.
Anyway, sorry for the off-topic rambling... "mailbox" as the queue name makes sense in my mind but open to other names :)
@@ -38,7 +38,7 @@ pub struct Model { | |||
} | |||
|
|||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] | |||
pub struct ShardId(usize); | |||
pub struct ShardId(pub usize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed, or just an artefact from previous code iterations? If it's not necessary, I'd prefer this to stay private.
Making it pub
is okay for our little model if it makes things easier. But generally speaking, it breaks the intended type safety. While the value is private, we can guarantee that ShardId
can only be created inside this file. So it's easy to check invariants on it. That's about halve the motivation for wrapping the usize
in a new type. It's the same pattern I used for QueueId
, TransactionId
, and ReceiptId
.
If you just need read-only access to the number, adding a simple getter method, or maybe impl From<ShardId> for usize {}
or even impl Deref for ShardId { type Target = usize }
could do the trick without breaking type properties but still allows reading the number with id.into()
or *id
, which seems just as convenient as id.0
.
This feature allows dumping the stats to a csv file. In a follow up PR I'll figure out how to plot this data conveniently. I implemented the framework and an example in the SimpleBackpressure strategy.
By default the model will add a single column
round
and populate it for each round.Each strategy needs to populate the head in the init method and populate the values in the compute_chunk method.
In the simple backpressure strategy the first few rows look like follows: