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

feat(consensus): add tracing instrumentation to consensus store #2546

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

itegulov
Copy link
Contributor

What ❔

This PR adds instrumentation to consensus store layer which should serve as a good starting point. Most other layers are heavily intertwined with the era-consensus codebase so we will need to introduce instrumentation there first.

Why ❔

Better visibility of what we spend our time on, including waiting time

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

@itegulov itegulov requested review from aakoshh and popzxc July 31, 2024 09:51
@pompon0 pompon0 self-requested a review July 31, 2024 11:21
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Nice, more visibility is surely going to help you understand what's happening. Keeping the code readable at the same time is a balancing act.

core/node/consensus/src/en.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
popzxc
popzxc previously approved these changes Aug 1, 2024
Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

core/node/consensus/src/storage/connection.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
core/node/consensus/src/storage/store.rs Outdated Show resolved Hide resolved
pompon0
pompon0 previously approved these changes Aug 1, 2024
@itegulov itegulov added this pull request to the merge queue Aug 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 1, 2024
@itegulov itegulov added this pull request to the merge queue Aug 1, 2024
Merged via the queue into main with commit 1e53940 Aug 1, 2024
47 checks passed
@itegulov itegulov deleted the daniyar/consensus/store-traces branch August 1, 2024 13:12
github-merge-queue bot pushed a commit that referenced this pull request Aug 2, 2024
🤖 I have created a release *beep* *boop*
---


##
[24.14.0](core-v24.13.0...core-v24.14.0)
(2024-08-01)


### Features

* Adding SLChainID
([#2547](#2547))
([656e830](656e830))
* **consensus:** add tracing instrumentation to consensus store
([#2546](#2546))
([1e53940](1e53940))
* Poll the main node for the next batch to sign (BFT-496)
([#2544](#2544))
([22cf820](22cf820))
* Support sending logs via OTLP
([#2556](#2556))
([1d206c0](1d206c0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: zksync-era-bot <[email protected]>
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.

4 participants