-
Notifications
You must be signed in to change notification settings - Fork 92
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(metric-stats): Emit accepted volume #3281
Conversation
58982c8
to
e3897dc
Compare
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.
Looks good to me!
As discussed:
- Slight preference for a global config org rollout rate.
- Preference for a thread-less "service" / facade for
MetricStatsService
.
/// and contain additional metadata, like the cardinality of a metric. | ||
#[derive(Clone, Debug)] | ||
pub struct MetricStats { | ||
config: Arc<Config>, |
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.
nit: Is config
only used to check processing_enabled
? If so, we could give MetricStats a noop variant instead, something like
enum MetricStats {
Disabled,
Enabled {
global_config: GlobalConfigHandle,
aggregator: Addr<Aggregator>,
}
}
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.
That makes dealing with self
awkward, since I have to pattern match it in every method.
29b3e41
to
328e79c
Compare
328e79c
to
821364d
Compare
821364d
to
131093a
Compare
Emits positive metric stats for the custom metrics namespace after submission to Kafka.
Only implements the happy path for metric stats and only volume.