Thanks for commenting, Matthias.
Why did you switch from passing in a `Collection` to a single metric?
With the changes in the implementation it became obvious that was the
necessary approach.
Since it seems the discussion has concluded, I'll start a vote now.
Thanks to all for the comments an
Thanks Bill. The KIP reads great. Overall LGTM.
Just one question out of curiosity. Why did you switch from passing in a
`Collection` to a single metric?
-Matthias
On 8/26/24 3:22 PM, Bill Bejeck wrote:
All,
I had originally planned to call for a vote, but during some early
implementation
All,
I had originally planned to call for a vote, but during some early
implementation work, I've discovered some changes are necessary.
First, we need a reciprocal method to remove a metric from subscriptions,
and I've updated the register method to take a single metric.
Thanks in advance for ta
Matthias and Andrew,
I've updated the KIP. I decided to not list the Kafka Stream metrics
supported as it would create a large amount of redundant information.
The types of supported metrics and behavior for unsupported metrics is
clearly defined.
Thanks,
Bill
On Thu, Aug 22, 2024 at 12:10 PM B
Hi Mathias and Andrew,
Thanks for taking the time to read and comment on the KIP. I'll address
each comment below:
MS1 - I agree that logging a WARN here should be sufficient. I'll update
the interface in KIP to the original `void` return type.
AS1 - I was considering throwing an exception, but
Hi Bill (and Matthias),
Thanks for the KIP. This looks like a valuable extension to KIP-714.
AS1: Personally, I think that registerMetricsForSubscription should return
void and not throw an exception if metrics push is not enabled. Otherwise,
you end up with an application whose behaviour is marke
Thanks for the KIP Bill.
Just for brainstorming: the newly added methods have `boolean` return
type. Is this necessary or would logging a WARN be sufficient? Or maybe
throw an exception if reporting is disabled?
As we intend to report KS metrics, I am wondering if we should talk
about this i
Hi Bill,
Thanks for addressing, looks good to me. Looking forward to implementation.
Regards,
Apoorv Mittal
On Wed, Aug 21, 2024 at 8:14 PM Bill Bejeck wrote:
> Hi Apoorv,
>
> Good call, I've updated the compatibility section with text about using
> older brokers.
>
> -Bill
>
> On Wed, Aug 21,
Hi Apoorv,
Good call, I've updated the compatibility section with text about using
older brokers.
-Bill
On Wed, Aug 21, 2024 at 1:30 PM Apoorv Mittal
wrote:
> Hi Bill,
> Thanks for the details. Sounds good to me.
>
> One last question:
>
> AM5: Do we need to capture any details around compatib
Hi Bill,
Thanks for the details. Sounds good to me.
One last question:
AM5: Do we need to capture any details around compatibility with older
brokers which don't support KIP-714 or when telemetry APIs are not enabled
on brokers (no configured receiver plugin)?
Regards,
Apoorv Mittal
On Tue, Au
Hi Apoorv,
Thank you for the discussion! Here are my responses to your follow-up
questions:
AM2: My apologies for the previous response; I misunderstood the question.
When executing the `registerMetricsForSubscription,` it is up to the
developer to provide all metrics that could possibly be in a
Hi Bill,
Thanks for the answers, some follow ups.
AM2: If we will only register subscribed metrics, as per the subscription
received from the broker, then are we going to call the
`registerMetricsForSubscription` post to GetTelemetrySubscription response
from the broker? Also how do we manage the
Hi Apoorv,
Thanks for reading the KIP and commenting. Here are my answers to your
comments/questions:
AM1: Good point; I'll update the Javadoc in the KIP to be more specific.
AM2: I'm not sure I follow, but IIUC, then yes, this method will only
subscribe telemetry metrics.
AM3: The `registerMe
Hi Bruno,
Thanks for the comments! Here are my responses:
BC1: Yes, I agree the name could use some work. My original intent was
that the user is adding additional metrics to the metrics already reported
by the given client. But the name you provided here works, so I'll update
the KIP accordin
Hi Bill,
Thanks for the KIP and it's great to see a way applications can also send
telemetry metrics over the KIP-714 pipeline.
A couple of questions:
AM1: The Javadoc for the method seems to be a bit vague, it talks about
"subscription" but do we need to be a bit more detailed regarding which
sub
Hi Bill,
Thanks for the KIP!
BC1
I find the name registerAdditionalMetrics() not specific enough. It is
not clear that the passed-in metrics are made available for
subscription. What about registerMetricsForSubscription()?
The Additional part is confusing in my opinion: additional to what?
B
Hi Lucas, thanks for the comments.
> LB1
Yes, I'll update the KIP signature to `? extends Metric`
>LB2
If `enable.metrics.push` is disabled, the metrics passed in are ignored.
Thinking about this now, I'm considering updating the return type of
`registerAdditionalMetrics` to a `boolean` from `voi
Hi Bill!
Thanks for the KIP! I like this solution, it's general enough for many
kinds of application.
LB1: Could we accept an instance of the Metric interface instead of
KafkaMetric? No strong reason to do it, it would just make the
interface slightly more generic.
LB2: What is the behavior is `
Hi all,
I would like to start a discussion thread on KIP-1076 to enable metrics
collection for applications that embed a Kafka client.
The KIP can be found here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1076%3A++Metrics+for+client+applications+KIP-714+extension
I look forward to the
19 matches
Mail list logo
