Skip to content
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

data_collection.rs stability improvements #656

Merged
merged 15 commits into from
Oct 27, 2023
Merged

Conversation

kinrezC
Copy link
Contributor

@kinrezC kinrezC commented Oct 26, 2023

  • Refactors data_collection.rs to avoid any race conditions. This PR makes the whole event logger process sync and adds a signal from the environment to indicate when the process should shut down.
  • Default data output is now json
  • Future improvements should involve adding new data output formats
  • Closes test: cargo test data_output fails #590

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #656 (e69eb77) into main (dc0ffd0) will decrease coverage by 0.09%.
Report is 28 commits behind head on main.
The diff coverage is 90.56%.

@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
- Coverage   60.76%   60.67%   -0.09%     
==========================================
  Files          26       26              
  Lines        5533     5551      +18     
==========================================
+ Hits         3362     3368       +6     
- Misses       2171     2183      +12     
Files Coverage Δ
arbiter-core/src/environment/errors.rs 0.00% <ø> (ø)
arbiter-core/src/middleware/mod.rs 83.21% <100.00%> (-0.03%) ⬇️
bin/bind.rs 53.65% <0.00%> (ø)
arbiter-core/src/environment/mod.rs 83.08% <84.61%> (-0.12%) ⬇️
arbiter-core/src/middleware/connection.rs 88.00% <76.92%> (-5.34%) ⬇️
arbiter-core/src/data_collection.rs 91.39% <94.87%> (-7.47%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Alexangelj Alexangelj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if I am getting this right on whats going on here now:

  • We made a new dedicated broadcast channel
  • We have that channel run on a separate normal thread
  • We push each event through this channel and store it in the same place so its output to the same file

while let Some(res) = set.join_next().await {
info!("task completed: {:?}", res);
let receiver = self.receiver.unwrap();
let dir = self.directory.unwrap_or("./out".into());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this overwrite if an existing directory is there? Might conflict with default foundry output which is out, if it does an it overwrites the foundry out that could make issues

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. @kinrezC let's change this do something like ./data_output

return Ok(());
} else {
if logs.is_none() {
panic!("This shouldn't happen, but we should probably make this an error. Somehow we got told to broadcast `None` logs!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol is this possible to hit?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't think so. I think if no logs, you actually broadcast vec![]. That could actually be optimized out...

tokio::fs::remove_dir_all("./test_output1").await.unwrap();

env.stop();
tokio::fs::remove_dir_all("./out").await.unwrap();
}

#[traced_test]
#[tokio::test(flavor = "multi_thread")]
async fn data_capture_output_validation() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good test 🙏

@@ -0,0 +1 @@
{"arbx":{"ApprovalFilter":[{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"}]},"arby":{"ApprovalFilter":[{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"},{"owner":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","spender":"0x2efdc9eecfee3a776209fcb8e9a83a6b221d74f5","amount":"0x1"}]},"lex":{"PriceChangeFilter":[{"price":"0xde0b6b3a7640000"},{"price":"0xde0b6b3a7640000"},{"price":"0xde0b6b3a7640000"},{"price":"0xde0b6b3a7640000"},{"price":"0xde0b6b3a7640000"}]}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay with the labels is nice

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And unraveling it is super clean!

// Grab the connection from the client and add a new event sender so that we
// have a distinct channel to now receive events over
let event_transmuted: EventTransmuted<Arc<RevmMiddleware>, RevmMiddleware, D> =
unsafe { transmute(event) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk what this does or why its here, maybe a comment?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, is the kinda same unsafe stuff we did with resolving pending transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets you access private fields

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexangelj if you're talking about the transmute, it is essentially what Matt said.

So you can take an Event<Arc<RevmMiddleware>, RevmMiddleware, D> and cast it into the EventTransmuted<Arc<RevmMiddleware, RevmMiddleware, D> which then gives us direct access to the fields of the struct.

Copy link
Collaborator

@0xJepsen 0xJepsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remarkable work on this, you guys are beasts.

@0xJepsen 0xJepsen merged commit dc7bb8c into main Oct 27, 2023
@0xJepsen 0xJepsen deleted the colin/fix-event-logger branch October 27, 2023 15:15
@github-actions github-actions bot mentioned this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: cargo test data_output fails
4 participants