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

[EASY] Improve logging for submitting transactions #3158

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

MartinquaXD
Copy link
Contributor

@MartinquaXD MartinquaXD commented Dec 11, 2024

Description

There was a wish to associate MEV blocker request logs with transactions sent by the driver. This is currently not possible for 2 reasons:

  1. there is no log right after submitting the tx so associating logs by timestamp is not possible
  2. the /settle handler spawns a new task to make sure transactions get cancelled (if needed) even when the autopilot terminates the request. Unfortunately how we pass around request_ids (which could also be used to associated these logs) is very convoluted and was overlooked here. Since the spawned task has its request_id variable not populated it generates a new id which does not match the original id.

Changes

  1. add log right after submitting a tx. Additional tracing information we get by default are request_id, solver and mempool type
  2. spawn /settle task in a way that preserves the original request_id

Reference slack thread

@MartinquaXD MartinquaXD requested a review from a team as a code owner December 11, 2024 07:33
@MartinquaXD MartinquaXD changed the title Add log for debugging tx submission [EASY] Add log for debugging tx submission Dec 11, 2024
@MartinquaXD MartinquaXD changed the title [EASY] Add log for debugging tx submission [EASY] Improve logging for submitting transactions Dec 11, 2024
@MartinquaXD
Copy link
Contributor Author

Requesting re-review due to non-trivial addition to the PR.

Copy link
Contributor

@m-lord-renkse m-lord-renkse left a comment

Choose a reason for hiding this comment

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

LGTM

if let Some(id) = get_task_local_storage() {
tokio::task::spawn(REQUEST_ID.scope(id, future))
} else {
tokio::task::spawn(future)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the request id is not found maybe we should at least log that, as the intention of the caller of spawn_task_with_request_id() function is not fulfilled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmh, maybe the function name is not great then. The expectation is not that there has to be a request id set. But rather to ensure that we'd use the same id if it is.

@MartinquaXD MartinquaXD enabled auto-merge (squash) December 11, 2024 09:13
@MartinquaXD MartinquaXD merged commit 2ad85a3 into main Dec 11, 2024
11 checks passed
@MartinquaXD MartinquaXD deleted the add-log branch December 11, 2024 09:18
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants