feat(sequencer): report deposit events #1447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Summary After sequencer relayer version update there was missed config update in chart this corrects.
…1192) ## Summary This PR introduces several types in `astria-telemetry` to support building and recording metrics in our other crates. ## Background There are a few reasons driving the design here: * I wanted to remove the third-party `metrics` crate as a direct dependency from all our crates except `astria-telemetry` so that its macros aren't available in our other crates. This precludes accidental addition of a new metric in the future which isn't properly initialized at app startup. * There's no possibility of a description for a given metric being ignored due to an accidental mismatch in the `metrics` macros being used (e.g. we previously had a case of `describe_gauge!` being used with a `counter!` meaning the description was ignored) * We can set buckets on a per-histogram basis * We can set up metrics in our black box tests without requiring an actual http server to be running. Configuring the `metrics` crate currently doesn't support either passing in an already-bound listener (where we could bind to port 0 and find out what actual port was used before giving the listener to `metrics`) or querying the running server in `metrics` to find out the port. This makes reliable testing using an actual http server essentially impossible. With the changes in this PR, the black box tests will retain a handle to the underlying exporter, meaning snapshots can be rendered which yield the same view of the metrics as if the http endpoint had been called. ## Changes * Added newtypes for `Counter`, `Gauge` and `Histogram`. * Added a `Metrics` trait, implemented by each of the crates' `Metrics` structs to support registering the metrics via `telemetry::Config::try_init`. * Added `metrics::ConfigBuilder` to support configuring and initializing metrics outside of `telemetry::Config::try_init` (for use in black box tests). * Added `metrics::RegisteringBuilder` to support registering individual metrics via the `Metrics` trait. To register a metric, this builder returns an appropriate factory, and the factory is then used potentially multiple times to register and return the individual metric with or without groups of labels applied. * Added `metrics::BucketBuilder` to support registering histogram buckets via the `Metrics` trait. This is unfortunately a separate builder since the `metrics` crate doesn't support registering buckets at the same point as registering the histograms. The former must happen via the `PrometheusBuilder`, but the latter can only be done once the `PrometheusBuilder` has been consumed and converted to a `PrometheusRecorder`. I added checks to ensure that buckets set via the `BucketBuilder` are all related to actual histograms registered later. * Changed the `telemetry::Config` setters to use a consistent `set_` naming convention and merged the two metrics-related setters into a single one, since now we require either both or neither. Note that these changes aren't mutually exclusive to using the raw `metrics` crate macros. So while we want to discourage usage in our own crates, dependencies using the macros will continue to function as before assuming we have configured metrics to use the global recorder. Using `telemetry::Config::try_init` does enable the global recorder, whereas we disable it in our black box tests so they don't conflict. ## Testing Manually checked metrics are still available right after startup, and manually checked the handle provided to black box tests functions as expected. However, proper testing of metrics should now be possible and I'd like to tackle that in follow-up PRs. ## Related Issues Closes #1147.
## Summary Adds smoke test for end to end Celestia > Astria > EVM > back to Astria > Celestia bridging ## Background Smoke tests r gud ## Changes * Combines work from withdraw smoke tests and ibc bridging tests ## Testing This is the PR for the testing --------- Co-authored-by: Jordan Oroshiba <jordan@astria.org>
Fraser999
approved these changes
Sep 11, 2024
Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>
github-actions
bot
requested review from
a team,
joroshiba and
SuperFluffy
as code owners
github-actions
bot
added
proto
ethanoroshiba
removed
proto
steezeburger
added a commit
that referenced
this pull request
Sep 23, 2024
* main: feat(sequencer): make mempool balance aware (#1408) chore(sequencer): change test addresses to versions with known private keys (#1487) chore(chart): update geth tag (#1485) feat(sequencer): report deposit events (#1447) feat(proto, core, sequencer)!: add traceability to rollup deposits (#1410) fix(bridge-withdrawer, cli, sequencer-client): migrate from `broadcast_tx_commit` to `broadcast_tx_sync` (#1376) fix(sequencer): add `end_block` to `app_execute_transaction_with_every_action_snapshot` (#1455) release: end of iteration release cuts (#1456) chore(charts): rollupName templates (#1458) chore(sequencer-relayer): Add instrumentation (#1375) feat(proto, core, sequencer)!: permit bech32 compatible addresses (#1425) chore: memoize `address_bytes` of verification key (#1444) chore(ci): include `ibc-bridge-test` in `docker` CI target (#1438) chore(charts): bump celestia versions (#1431)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added creation of ABCI deposit events and record them to the state to be included in
ExecTxResult.Background
Deposit events were previously only stored app-side in
SequencerBlock. This will allow for outside observation of the deposit events.Changes
Testing
Added test to ensure deposit events are correctly recorded and returned by
state.apply()Related Issues
closes #1440