KAFKA-9960: implement KIP-606 to add metadata context to MetricsReporter #8691
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.common.metrics; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * A implementation of MetricsContext, it encapsulates required metrics context properties for Kafka services and clients | ||
| */ | ||
| public class KafkaMetricsContext implements MetricsContext { | ||
| /** | ||
| * Client or Service's metadata map. | ||
| */ | ||
| private Map<String, String> metadata = new HashMap<>(); | ||
|
mumrah marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * Create a MetricsContext with namespace, no service or client properties | ||
| * @param namespace value for _namespace key | ||
| */ | ||
| public KafkaMetricsContext(String namespace) { | ||
| this(namespace, new HashMap<>()); | ||
| } | ||
|
|
||
| /** | ||
| * Create a MetricsContext with namespace, service or client properties | ||
| * @param namespace value for _namespace key | ||
| * @param metadata metadata additional entries to add to the context. | ||
| * values will be converted to string using Object.toString() | ||
| */ | ||
| public KafkaMetricsContext(String namespace, Map<String, ?> metadata) { | ||
| this.metadata.put(MetricsContext.NAMESPACE, namespace); | ||
| metadata.forEach((key, value) -> this.metadata.put(key, value.toString())); | ||
|
Contributor
There was a problem hiding this comment. Use
Member
There was a problem hiding this comment. I think we it's ok for the component that owns the reporter to take precedence over the labels passed from upstream. We did not specify the behavior in the KIP, so implementations should use namespacing of labels to avoid this. If in practice we find this behavior is less desirable, we can file a follow-on KIP, since the interface is till evolving.
Contributor
There was a problem hiding this comment. Mostly I'm concerned about the case where some composite may share similar labels to the underlying client it manages. If we allow the downstream client to overwrite such a label we will lose a portion of the upstream components context.
Member
There was a problem hiding this comment. the client currently only injects the labels passed in via client properties, so that wouldn't happen |
||
|
|
||
| } | ||
|
|
||
| public Map<String, String> metadata() { | ||
| return Collections.unmodifiableMap(metadata); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.common.metrics; | ||
|
|
||
| import org.apache.kafka.common.annotation.InterfaceStability; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * MetricsContext encapsulates additional metadata about metrics exposed via a | ||
| * {@link org.apache.kafka.common.metrics.MetricsReporter} | ||
| * | ||
| * The metadata map provides following information: | ||
| * - a <code>_namespace</node> field indicating the component exposing metrics | ||
| * e.g. kafka.server, kafka.consumer | ||
| * {@link JmxReporter} uses this as prefix for mbean names | ||
| * | ||
| * - for clients and streams libraries: any freeform fields passed in via | ||
| * client properties in the form of `metrics.context.<key>=<value> | ||
| * | ||
| * - for kafka brokers: kafka.broker.id, kafka.cluster.id | ||
| * - for connect workers: connect.kafka.cluster.id, connect.group.id | ||
| */ | ||
| @InterfaceStability.Evolving | ||
| public interface MetricsContext { | ||
|
Member
There was a problem hiding this comment. Looking through the PR, it seems that MetricsContext is a short lived object used to pass values to the MetricReporters as they are constructed. Since the usage appears to be write-once, it might be better to expose a subset of Map rather than the full thing. E.g., If we think this might evolve into a mutable long-lived object, then a Map is probably better. Just a thought.
Member
There was a problem hiding this comment. I think having an interface gives us more flexibility to evolve the API, without breaking backwards compatibility.
Member
There was a problem hiding this comment. I wasn't suggesting that we eliminate the interface, I definitely think having one is a good choice (for the reasons you mentioned). What I meant was in this interface, we expose a Map as the collection of metrics tags/labels. But since it appears that the usage is intended to be read-only, maybe a Map isn't the best choice. Here's what I was thinking: interface MetricsContext {
String namespace();
String get(String field);
Collection<String> fields();
}(included the namespace suggestion from my other comment as well).
Member
There was a problem hiding this comment. I think keeping a Map interface makes it more convenient to work with. It gives you all the helper methods and streaming interfaces rather than having to hand-roll those things for someone consuming the api. |
||
| /* predefined fields */ | ||
| String NAMESPACE = "_namespace"; // metrics namespace, formerly jmx prefix | ||
|
Member
There was a problem hiding this comment. Should we define this as a field on the interface?
Member
There was a problem hiding this comment. I think the interface is a natural way to expose predefined constants an API might need. I don't see a need to have a separate class for this yet.
Member
There was a problem hiding this comment. Sorry, I shouldn't have said "field" since that implies a concrete class. See above comment above for an example of what I meant.
Member
There was a problem hiding this comment. I'm not convinced we should give namespace a special status over other fields, it just happens to be the only one we currently define by default for backwards compatibility reasons. If we find ourselves adding more of those, I agreee it would be worth revisiting how we expose pre-defined fields. |
||
|
|
||
| /** | ||
| * Returns metadata fields | ||
| */ | ||
| Map<String, String> metadata(); | ||
|
Member
There was a problem hiding this comment. Metadata is very overloaded, can we think of a different name here?
Member
There was a problem hiding this comment. any suggestions? maybe
Member
There was a problem hiding this comment. ok, I updated the KIP to reflect this change.
Contributor
Author
There was a problem hiding this comment. Updated code to reflect KIP changes |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| import java.util.Set; | ||
|
|
||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.annotation.InterfaceStability; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
|
|
||
| /** | ||
|
|
@@ -65,4 +66,13 @@ default void validateReconfiguration(Map<String, ?> configs) throws ConfigExcept | |
| default void reconfigure(Map<String, ?> configs) { | ||
| } | ||
|
|
||
| /** | ||
| * Callback method providing context metadata for the | ||
|
mumrah marked this conversation as resolved.
Outdated
|
||
| * service or library exposing metrics | ||
| * | ||
| * @param metricsContext the metric context | ||
| */ | ||
| @InterfaceStability.Evolving | ||
| default void contextChange(MetricsContext metricsContext) { | ||
|
Member
Contributor
There was a problem hiding this comment. I really don't have a strong preference. Past tense is a little odd, but I think
Contributor
There was a problem hiding this comment. It would also be good to identify when this is called relative to other methods. For example, it is always called before
Member
There was a problem hiding this comment. due to the way jmxreporter is initialized in Kafka today, it already gets called both before and after
Contributor
Author
There was a problem hiding this comment. It can be called multiple times. Not sure if we should mention that in Javadoc, other methods in this class we are not mention if it can be called multiple times even though they can be called multiple times.
Contributor
There was a problem hiding this comment. If there is no clear call pattern, then it's fine to not say anything. However, If that call pattern is true, then I think we should document it. If it's also true it can be called later, then mention this as well. For example, the JavaDoc text on
WDYT?
Contributor
Author
There was a problem hiding this comment. Sounds good. Updated javadoc. |
||
| } | ||
| } | ||
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.
@xvrl can we remove the explicit construction of JmxReporter and rely on the plugin loading mechanism after we remove this non-zero-arg constructor? Maybe something for 3.0?
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, I agree. We'll have to decide whether to make the config include this reporter by default or so something else. There are some backwards compatibility implications, but probably better to have a separate discussion for this.
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.
Sounds good, thanks 👍