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.

Sign up for GitHub

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

Jump to bottom

chore(sequencer): Add instrumentation #1368

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

chore(sequencer): Add instrumentation #1368

wants to merge 17 commits into from

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Aug 14, 2024 •
edited
Loading

Summary

Added instrumentation to astria-sequencer

Background

Adding instrumentation to all async calls will aid in tracing since spans will be emitted even if no events happen under them.

Changes

  • Disabled clippy::blocks_in_conditions lint
  • Added instrumentation to all async function calls that are not long-lived.
  • Removed instrumentation from long-running functions.
  • Minor refactoring of two functions to get rid of clippy exceptions and move logging within spans

Related Issues

closes #1321

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Aug 14, 2024
@ethanoroshiba ethanoroshiba added code-quality observability observability, tracing, metrics labels Aug 15, 2024
@github-actions github-actions bot added the ci issues that are related to ci and github workflows label Aug 15, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review August 15, 2024 19:37
@ethanoroshiba ethanoroshiba requested review from a team as code owners August 15, 2024 19:37
Fraser999
Fraser999 reviewed Aug 19, 2024
View reviewed changes
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

I think we should be using err(Debug) throughout this PR since we're dealing with anyhow::Results, and they are usually fairly unhelpful if just Display is used.

Base automatically changed from gh-1318 to main August 20, 2024 11:42
@ethanoroshiba ethanoroshiba force-pushed the ENG-670/add_sequencer_instrumentation branch from a8c9bfa to cd15920 Compare August 21, 2024 14:00
@ethanoroshiba
Copy link
Contributor Author

I think we should be using err(Debug) throughout this PR since we're dealing with anyhow::Results, and they are usually fairly unhelpful if just Display is used.

Reverted Debug instances for this PR, will make a followup to transition to eyre instead of anyhow

This was referenced Aug 21, 2024
Closed
Merged
This was referenced Sep 19, 2024
Closed
Merged
SuperFluffy
SuperFluffy reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Overall good PR, but I would like to expand this PR, adding fields where needed.

When writing #1495 I started adding fields to the spans because without them the generated err events will be hard to understand.

This will inevitably create tension when reading logs on stdout, but to understand error context there is no way around.

crates/astria-sequencer/src/accounts/query.rs Show resolved Hide resolved
crates/astria-sequencer/src/accounts/query.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/accounts/query.rs Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
crates/astria-sequencer/src/accounts/state_ext.rs Outdated
@@ -208,7 +208,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt {
Ok(balance)
}

#[instrument(skip_all)]
#[instrument(skip_all, err)]
Copy link
Member

Choose a reason for hiding this comment

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

Note on this T: AddressBytes and all methods that contain a similar argument: in #1495 I have added logic to display the address. It would be very useful to get that context here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to add similar fields wherever valuable in 2b30bf2 :)

github-merge-queue bot pushed a commit that referenced this pull request Sep 25, 2024
@SuperFluffy
chore(ci): bump rust toolchain to 1.81 (#1523)
4478cb6 
## Summary
Updates the rust toolchain used for all rust releated github jobs to and
the containerfile to 1.81.0.

## Background
Rust [1.81 was
released](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html) and
comes with a load of niceties that we would like to use. Among them of
immediate interest are:

1. lint reasons (#1521)
2. fixes `clippy::blocks_in_conditions` lint triggering on proc macros
like `tracing::instrument` (which
#1368 needed to work around)
3. lazy lock and lazy cell (to replace once_cell) in 1.80
(#1520)
4. checked cfg names (also 1.80 which flags a few issues in our code and
should be immediately available).

Note that this is orthgonal to setting the MSRV in individual Astria
crates.

## Changes
- Updates all github workflows to use the `1.81.0` toolchain (except for
rustfmt and dylint, which need nightlies, respectively).
- Fix all new clippy warnings.
- Upgraded to the most recent dylint to make it compile again
- Upgrade the single containerfile to use `rust:1.81-bookwarm` as the
base image

## Related Issues
Closes #1522
@ethanoroshiba ethanoroshiba removed request for a team and joroshiba September 25, 2024 13:56
bharath-123 pushed a commit that referenced this pull request Sep 26, 2024
@SuperFluffy @bharath-123
chore(ci): bump rust toolchain to 1.81 (#1523)
347152d 
## Summary
Updates the rust toolchain used for all rust releated github jobs to and
the containerfile to 1.81.0.

## Background
Rust [1.81 was
released](https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html) and
comes with a load of niceties that we would like to use. Among them of
immediate interest are:

1. lint reasons (#1521)
2. fixes `clippy::blocks_in_conditions` lint triggering on proc macros
like `tracing::instrument` (which
#1368 needed to work around)
3. lazy lock and lazy cell (to replace once_cell) in 1.80
(#1520)
4. checked cfg names (also 1.80 which flags a few issues in our code and
should be immediately available).

Note that this is orthgonal to setting the MSRV in individual Astria
crates.

## Changes
- Updates all github workflows to use the `1.81.0` toolchain (except for
rustfmt and dylint, which need nightlies, respectively).
- Fix all new clippy warnings.
- Upgraded to the most recent dylint to make it compile again
- Upgrade the single containerfile to use `rust:1.81-bookwarm` as the
base image

## Related Issues
Closes #1522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperFluffy SuperFluffy SuperFluffy left review comments

@noot noot Awaiting requested review from noot noot is a code owner automatically assigned from astriaorg/rust-reviewers

@Fraser999 Fraser999 Awaiting requested review from Fraser999

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
ci issues that are related to ci and github workflows code-quality observability observability, tracing, metrics sequencer pertaining to the astria-sequencer crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: add instrumentation on all async calls
3 participants
@ethanoroshiba @Fraser999 @SuperFluffy