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.
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 #760base: main
Are you sure you want to change the base?
feat:
PipelineMetrics
common scheme,no-op
version #760Changes from 3 commits
32fd034
a824ce3
04f79ba
5b3cced
5f61eca
82591bc
b27315d
014f073
4a0d6af
5ca014c
25c54ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Check warning on line 19 in crates/derive/src/metrics/mod.rs
Codecov / codecov/patch
crates/derive/src/metrics/mod.rs#L17-L19