Hi Kevin, thanks for the KIP. I have a couple of questions/considerations. 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.
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). On Thu, Mar 6, 2025 at 8: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 > >>> > >>
