-
Notifications
You must be signed in to change notification settings - Fork 52
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: PipelineMetrics
common scheme, no-op
version
#760
Open
steph-rs
wants to merge
11
commits into
op-rs:main
Choose a base branch
from
steph-rs:feat/derive-pipeline-metrics-noop
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
32fd034
feat: `PipelineMetrics` common scheme, metrics for `DerivationPipeline`
steph-rs a824ce3
fix: tests
steph-rs 04f79ba
chore: fmt
steph-rs 5b3cced
fix: `.idea` dirs
steph-rs 5f61eca
fix: `DerivationPipelineMetrics` changed
steph-rs 82591bc
fix: `DerivationPipeline` metrics changes
steph-rs b27315d
feat: add `AttributesQueueMetrics`, refactoring
steph-rs 014f073
feat: add no-op metrics for a few stages
steph-rs 4a0d6af
feat: add no-op metrics for `ChannelProvider` stage
steph-rs 5ca014c
feat: add no-op metrics for `ChannelReader`, `BatchStream`, `BatchQue…
steph-rs 25c54ce
feat: add no-op metrics for `AttributesQueue` stage
steph-rs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,6 @@ monorepo/ | |
|
||
# Environment Variables | ||
.env | ||
|
||
# JetBrains | ||
**/.idea/ |
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
//! Metrics for the derivation pipeline. | ||
|
||
mod noop; | ||
|
||
use crate::{ | ||
errors::PipelineErrorKind, | ||
pipeline::Signal, | ||
traits::{ | ||
AttributesQueueMetrics, BatchQueueMetrics, BatchStreamMetrics, ChannelProviderMetrics, | ||
ChannelReaderMetrics, DerivationPipelineMetrics, FrameQueueMetrics, L1RetrievalMetrics, | ||
L1TraversalMetrics, StepResult, | ||
}, | ||
}; | ||
use alloc::sync::Arc; | ||
use core::fmt::Debug; | ||
|
||
/// Composite metrics struct containing metrics for all stages. | ||
#[derive(Clone)] | ||
pub struct PipelineMetrics { | ||
pub(crate) derivation_pipeline_metrics: Arc<dyn DerivationPipelineMetrics + Send + Sync>, | ||
pub(crate) l1_traversal_metrics: Arc<dyn L1TraversalMetrics + Send + Sync>, | ||
pub(crate) l1_retrieval_metrics: Arc<dyn L1RetrievalMetrics + Send + Sync>, | ||
pub(crate) frame_queue_metrics: Arc<dyn FrameQueueMetrics + Send + Sync>, | ||
pub(crate) channel_provider_metrics: Arc<dyn ChannelProviderMetrics + Send + Sync>, | ||
pub(crate) channel_reader_metrics: Arc<dyn ChannelReaderMetrics + Send + Sync>, | ||
pub(crate) batch_stream_metrics: Arc<dyn BatchStreamMetrics + Send + Sync>, | ||
pub(crate) batch_queue_metrics: Arc<dyn BatchQueueMetrics + Send + Sync>, | ||
pub(crate) atrirbutes_queue_metrics: Arc<dyn AttributesQueueMetrics + Send + Sync>, | ||
} | ||
|
||
impl Debug for PipelineMetrics { | ||
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
f.debug_struct("PipelineMetrics").finish() | ||
} | ||
} | ||
|
||
impl DerivationPipelineMetrics for PipelineMetrics { | ||
fn record_step_result(&self, result: &StepResult) { | ||
self.derivation_pipeline_metrics.record_step_result(result) | ||
} | ||
|
||
fn record_signal(&self, signal: &Signal) { | ||
self.derivation_pipeline_metrics.record_signal(signal) | ||
} | ||
} | ||
|
||
impl L1TraversalMetrics for PipelineMetrics { | ||
fn record_block_processed(&self, block_number: u64) { | ||
self.l1_traversal_metrics.record_block_processed(block_number) | ||
} | ||
|
||
fn record_system_config_update(&self) { | ||
self.l1_traversal_metrics.record_system_config_update() | ||
} | ||
|
||
fn record_reorg_detected(&self) { | ||
self.l1_traversal_metrics.record_reorg_detected() | ||
} | ||
|
||
fn record_holocene_activation(&self) { | ||
self.l1_traversal_metrics.record_holocene_activation() | ||
} | ||
} | ||
|
||
impl L1RetrievalMetrics for PipelineMetrics { | ||
fn record_data_fetch_attempt(&self, block_number: u64) { | ||
self.l1_retrieval_metrics.record_data_fetch_attempt(block_number) | ||
} | ||
|
||
fn record_data_fetch_success(&self, block_number: u64) { | ||
self.l1_retrieval_metrics.record_data_fetch_success(block_number) | ||
} | ||
|
||
fn record_data_fetch_failure(&self, block_number: u64, error: &PipelineErrorKind) { | ||
self.l1_retrieval_metrics.record_data_fetch_failure(block_number, error) | ||
} | ||
|
||
fn record_block_processed(&self, block_number: u64) { | ||
self.l1_retrieval_metrics.record_block_processed(block_number) | ||
} | ||
} | ||
|
||
impl FrameQueueMetrics for PipelineMetrics { | ||
fn record_frames_decoded(&self, count: usize) { | ||
self.frame_queue_metrics.record_frames_decoded(count) | ||
} | ||
|
||
fn record_frames_dropped(&self, count: usize) { | ||
self.frame_queue_metrics.record_frames_dropped(count) | ||
} | ||
|
||
fn record_frames_queued(&self, count: usize) { | ||
self.frame_queue_metrics.record_frames_queued(count) | ||
} | ||
|
||
fn record_load_frames_attempt(&self) { | ||
self.frame_queue_metrics.record_load_frames_attempt() | ||
} | ||
} | ||
|
||
impl ChannelProviderMetrics for PipelineMetrics { | ||
fn record_stage_transition(&self, from: &str, to: &str) { | ||
self.channel_provider_metrics.record_stage_transition(from, to) | ||
} | ||
|
||
fn record_data_item_provided(&self) { | ||
self.channel_provider_metrics.record_data_item_provided() | ||
} | ||
} | ||
|
||
impl ChannelReaderMetrics for PipelineMetrics { | ||
fn record_batch_read(&self) { | ||
self.channel_reader_metrics.record_batch_read() | ||
} | ||
|
||
fn record_channel_flushed(&self) { | ||
self.channel_reader_metrics.record_channel_flushed() | ||
} | ||
} | ||
|
||
impl BatchStreamMetrics for PipelineMetrics { | ||
fn record_batch_processed(&self) { | ||
self.batch_stream_metrics.record_batch_processed() | ||
} | ||
|
||
fn record_span_batch_accepted(&self) { | ||
self.batch_stream_metrics.record_span_batch_accepted() | ||
} | ||
|
||
fn record_span_batch_dropped(&self) { | ||
self.batch_stream_metrics.record_span_batch_dropped() | ||
} | ||
|
||
fn record_buffer_size(&self, size: usize) { | ||
self.batch_stream_metrics.record_buffer_size(size) | ||
} | ||
} | ||
|
||
impl BatchQueueMetrics for PipelineMetrics { | ||
fn record_batches_queued(&self, count: usize) { | ||
self.batch_queue_metrics.record_batches_queued(count) | ||
} | ||
|
||
fn record_batch_dropped(&self) { | ||
self.batch_queue_metrics.record_batch_dropped() | ||
} | ||
|
||
fn record_epoch_advanced(&self, epoch: u64) { | ||
self.batch_queue_metrics.record_epoch_advanced(epoch) | ||
} | ||
} | ||
|
||
impl AttributesQueueMetrics for PipelineMetrics { | ||
fn record_attributes_created(&self) { | ||
self.atrirbutes_queue_metrics.record_attributes_created() | ||
} | ||
|
||
fn record_batch_loaded(&self) { | ||
self.atrirbutes_queue_metrics.record_batch_loaded() | ||
} | ||
|
||
fn record_attributes_creation_failure(&self) { | ||
self.atrirbutes_queue_metrics.record_attributes_creation_failure() | ||
} | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the
Arc
here - it can be placed around thePipelineMetrics
.If we do this, we can then make a
PipelineMetrics::NOOP
const
. We should use a generic here bound toDerivationPipelineMetrics + Send + Sync
.e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we have to use generics for each stage like
I was trying to avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to do that btw, but how should other metrics be implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you're saying... Let me think about this a little bit and get back to you. What you're currently proposing is to place the various stage metrics inside
PipelineMetrics
and then each stage holds aPipelineMetrics
which implements any metrics trait required through the internal field for that stage? Is that right?e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, exactly
but I think there's a typo in first block:
and then we could use
self.metrics.some_metrics_method()
I'm not sure about this idea, but it can help us easily add other versions as
no-op
, orprometheus
. I thought this was a problem we wanted to solve.Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I thought you're intention of placing the various metric types inside
PipelineMetrics
was to be able to clone PipelineMetrics (doingArc::clone
for each internalArc<dyn ..>
field) and then passing it as a concrete type to each stage. This way, you could just avoid having another generic on each stage, and instead just import the trait that is implemented on thePipelineMetrics
type.This is why in my first block above I specific
PipelineMetrics
explicitly. And yes like you said - the problem to solve is having different metrics. sincePipelineMetrics
itself has dynamic fields, you can swap in a prometheus, or no-op, or whatever individual metrics easily, without any api changes.Let me know if this makes sense. Happy to open a draft PR into this branch to show you a diff of what I'm getting at here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @refcell!
Could you please check the fixes I just pushed and the way I work with
AttributesQueueMetrics
. If it's okay with you, I'll add other no-op metrics.But since I don't really see the whole picture of what we actually need in metrics, I will ask you to define the exact methods we want. It will be much appreciated.