Thanks for the comments Federico. > If I understand correctly unfenced == active. In the code we always > use the term active, so I think it would be better to use that for the > state 0 description. I've updated the KIP description to refer to "active".
> You propose creating per-broker metrics indicating their state > (BrokerRegistrationState.kafka-X). Can't these new metrics be used to > derive broker counters in whatever monitor tool you decide to use? I > mean, we wouldn't need to store and provide > ControlledShutdownBrokerCount (proposed), FencedBrokerCount > (existing), ActiveBrokerCount (existing). Yes, we can use this new metric to derive broker counters, but it's just more complicated for the operator to implement. Also, I don't think it's a huge issue that there's a slight redundancy here, since deleting the existing metrics will lead to compatibility issues with current monitoring setups. On Mon, Apr 14, 2025 at 12:25 PM Kevin Wu <kevin.wu2...@gmail.com> wrote: > Thanks for the comments Jose. > For 1 and 2, I've changed the naming of the metrics to follow your > suggestion of tags/attributes. For 3, I made a note as to why we need the > maximum. Basically, it's because the map that contains broker contact times > we're using as the source for these metrics removes entries when a broker > is fenced. Therefore, we need some default value when the entry doesn't > exist for the broker, but it is still registered. > > Thanks, > Kevin > > > Thanks for the improvement Kevin. I got a chance to look at the KIP. > > > > 1. > kafka.controller:type=KafkaController,name=BrokerRegistrationState.kafka-X > > > > Can we use tags or attributes instead of different names? For example, > > how about > kafka.controller:type=KafkaController,name=BrokerRegistrationState,broker=X > > where X is the node id? > > > > 2. > kafka.controller:type=KafkaController,name=TimeSinceLastHeartbeatReceivedMs.kafka-X > > > > Same here, did you consider using tags or attributes for the node id? > > > > 3. For the metrics > > > kafka.controller:type=KafkaController,name=TimeSinceLastHeartbeatReceivedMs.kafka-X, > > you mentioned that you will limit the value to the heartbeat timeout. > > Why? Wouldn't it be a useful report the entire time since the last > > heartbeat? That is more information instead of just reporting the > > value up to the heartbeat timeout. > > > > Thanks, > > -- > > -José > > > > On Thu, Mar 6, 2025 at 1:58 PM Kevin Wu <kevin.wu2...@gmail.com> wrote: > >> That's an interesting idea. However, I think that's going to be messy and >>> difficult for people to use. For example, how would you set up Grafana or >>> Datadog to use this? The string could also get extremely long (imagine 1000 >>> brokers all in startup.) >> >> Hmm... Yeah from what I've read so far setting this up might be kind of >> challenging. I'm not seeing that OTEL supports gauges for string values. >> >> I'm still a little confused as to why having a per-broker metric to >> expose its state is preferred, but I think this is at least part of the >> reason? When drafting this KIP, I was only really considering the scenarios >> of the broker's initial metadata load during startup and their controlled >> shutdown, which my proposed metrics would cover. However, there are a lot >> of other scenarios with fenced brokers which have already started up that >> the existing fencedBrokers metric doesn't really give enough information >> about from the controller-side, since it just reports the number. For these >> scenarios, I don't think my proposed startup/shutdown focused metrics would >> be very useful. >> I'm on board with the proposed per-broker metric that exposes its state. >> I think it would be helpful to enumerate some specific cases though for the >> KIP. >> >> On Thu, Feb 27, 2025 at 2:19 PM Kevin Wu <kevin.wu2...@gmail.com> wrote: >> >>> I guess my concern is that the time-based metrics would reset to 0 on >>>> every failover (if I understand the proposed implementation correctly). >>>> That seems likely to create confusion. >>> >>> Yeah that makes sense to me. I'm fine with moving towards the approach >>> of either (since I don't think we need both): >>> >>> - Exposing the number of brokers in 1. startup, 2. fenced (what we >>> have now), and 3. in controlled shutdown >>> - Exposing a per-broker metric reflecting the state of the broker >>> (more on this below). >>> >>> I think it would be useful to have a state for each broker exposed as a >>>> metric. I can think of a lot of scenarios where this would be useful to >>>> have. I don't think we should have more than one metric per broker though, >>>> if we can help it. >>> >>> Instead of having exactly a per-broker metric which exposes a number >>> that maps to a state (0, 1, 2, and 3), what if we expose 4 metrics whose >>> values are a comma-delimited string of the brokers in those states. >>> Something along the lines of: >>> >>> - Metric: name = BrokersNotRegistered, value = "kafka-1" >>> - Metric: name = BrokersRegisteredAndNeverUnfenced, value = "kafka-2" >>> - Metric: name = BrokersRegisteredAndFenced, value = >>> "kafka-2,kafka-3" >>> - Metric: name = BrokersRegisteredRegisteredAndUnfenced, value = >>> "kafka-4,kafka-5" >>> >>> I guess there will be overlap between the second and third metrics, but >>> there do exist metrics that expose `Gauge<String>` values. >>> >>> On Tue, Feb 25, 2025 at 4:12 PM Kevin Wu <kevin.wu2...@gmail.com> wrote: >>> >>>> Hey Colin, >>>> >>>> Thanks for the review. >>>> >>>> Regarding the metrics that reflect times: my initial thinking was to >>>> indeed have these be "soft state", which would be reset when a controller >>>> failover happens. I'm not sure if it's a big issue if these values get >>>> reset though, since a controller failover means brokers in startup would >>>> need to register again to the new controller anyways. Since what we're >>>> trying to monitor with these metrics is the broker's startup and shutdown >>>> statuses from the controller's view, my thinking was that exposing this >>>> soft state would be appropriate. >>>> >>>> There exist metrics that expose other soft state of the controller in >>>> `QuorumControllerMetrics.java`, and I thought the proposed metrics here >>>> would fit with what exists there. If instead we're updating these metrics >>>> based on the metadata log events for registration changes, it looks like >>>> `ControllerMetadataMetrics` has a `FencedBrokerCount` metric, and I guess >>>> we could add a `ControlledShutdownBrokerCount`. For specifically tracking >>>> brokers in their initial startup fencing using the log events, I'm not >>>> totally sure as of now how we can actually do this from only the >>>> information in `BrokerRegistration`. I guess we know a broker is undergoing >>>> startup when it's fenced and has an `incarnationId` the controller hasn't >>>> seen before in the log? >>>> >>>> Regarding the per-broker metrics, what are your thoughts about the >>>> metric cardinality of this? There was some discussion about having a >>>> startup/shutdown time per-broker and I pushed back against it because the >>>> number of metrics we expose as a result is the number of brokers in the >>>> cluster. Additionally, I don't think the controller can know of a live >>>> broker that has not attempted to register yet in order to make a metric for >>>> it and assign it a value of 0. Is a value of 0 for brokers that shutdown? >>>> In that case, doesn't that make the metric cardinality worse? I think if we >>>> decide to go that route we should only have states 1, 2, and 3. >>>> >>>> Best, >>>> Kevin Wu >>>> >>>> On Mon, Jan 27, 2025 at 12:56 PM Kevin Wu <kevin.wu2...@gmail.com> >>>> wrote: >>>> >>>>> Hey all, >>>>> >>>>> I posted a KIP to monitor broker startup and controlled shutdown on >>>>> the controller-side. Here's the link: >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1131%3A+Controller-side+monitoring+for+broker+shutdown+and+startup >>>>> >>>>> Best, >>>>> Kevin Wu >>>>> >>>>
