-
Notifications
You must be signed in to change notification settings - Fork 1k
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: add interface for metrics reporter #7788
feat: add interface for metrics reporter #7788
Conversation
The added interface specify how to report metrics to a metrics framework. The implementations of this interface specify how to report to a specific metrics framework (e.g. JMX or Confluent's Telemetry pipeline)
@confluentinc It looks like @cadonna just signed our Contributor License Agreement. 👍 Always at your service, clabot |
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.
Thanks @cadonna, lgtm. Left a few questions to help my understanding
@@ -0,0 +1,112 @@ | |||
/* | |||
* Copyright 2018 Confluent Inc. |
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: 2021?
final Map<String, String> tags | ||
) { | ||
this.name = Objects.requireNonNull(name, "name"); | ||
this.time = time; |
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.
Just curious, why don't you require non-null for time and value?
* | ||
* @param dataPointSupplier supplier of the list of data points | ||
*/ | ||
void report(Supplier<List<DataPoint>> dataPointSupplier); |
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.
Just to check my understanding - will there be a telemetry + jmx reporter that implement this interface / this method? and this method will be called from the metrics skeleton?
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.
Yes, there will be a Telemetry and a JMX implementation. On-prem the JMX reporter will be instantiated and in ccloud the telemetry reporter. Method report()
will be called from the metrics skeleton.
*/ | ||
class DataPoint { | ||
private final String name; | ||
private final Instant time; |
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.
@cadonna sorry I didn't catch this earlier - but why did you pick Instant
here? JW because right now the metrics reporter skeleton usese Time
/ long
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.
@cadonna just for my own understanding: this framework would not be used for dd metrics / healthcheck metrics, right? It's only gonna used for telemetry in ccloud.
Description
The added interface specifies how to report metrics to a metrics
framework. The implementations of this interface specify how to
report to a specific metrics framework (e.g. JMX or Confluent's
Telemetry pipeline)
Testing done
No tests at this point since it is a plain interface.
Reviewer checklist