Skip to content
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.

Sign up for GitHub

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

KAFKA-17516: Synonyms for client metrics configs #17264

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

KAFKA-17516: Synonyms for client metrics configs #17264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
84ab266
KAFKA-17516: Synonyms for client metrics configs
AndrewJSchofield Sep 24, 2024
4df9cdf
Test failure
AndrewJSchofield Sep 26, 2024
4252dc9
Merge remote-tracking branch 'upstream/trunk' into KAFKA-17516
AndrewJSchofield Oct 2, 2024
ec38702
Checkstyle fix
AndrewJSchofield Oct 3, 2024
9aee5eb
Merge remote-tracking branch 'upstream/trunk' into KAFKA-17516
AndrewJSchofield Oct 7, 2024
a2254c1
Merge upstream/trunk
AndrewJSchofield Nov 11, 2024
80c8d9d
Merge remote-tracking branch 'upstream/trunk' into KAFKA-17516
AndrewJSchofield Jan 20, 2025
f21f162
Correction after merge
AndrewJSchofield Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions core/src/main/scala/kafka/server/ConfigHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ import org.apache.kafka.common.resource.Resource.CLUSTER_NAME
import org.apache.kafka.common.resource.ResourceType.{CLUSTER, GROUP, TOPIC}
import org.apache.kafka.coordinator.group.GroupConfig
import org.apache.kafka.server.config.ServerTopicConfigSynonyms
import org.apache.kafka.server.metrics.ClientMetricsConfigs
import org.apache.kafka.storage.internals.log.LogConfig

import scala.collection.mutable.ListBuffer
import scala.collection.{Map, mutable}
import scala.jdk.CollectionConverters._
import scala.jdk.OptionConverters.RichOptional
Expand Down Expand Up @@ -136,21 +136,12 @@ class ConfigHelper(metadataCache: MetadataCache, config: KafkaConfig, configRepo
.setIsSensitive(false).setReadOnly(false).setSynonyms(List.empty.asJava))

case ConfigResource.Type.CLIENT_METRICS =>
val subscriptionName = resource.resourceName
if (subscriptionName == null || subscriptionName.isEmpty) {
if (resource.resourceName == null || resource.resourceName.isEmpty) {
throw new InvalidRequestException("Client metrics subscription name must not be empty")
} else {
val entityProps = configRepository.config(new ConfigResource(ConfigResource.Type.CLIENT_METRICS, subscriptionName))
val configEntries = new ListBuffer[DescribeConfigsResponseData.DescribeConfigsResourceResult]()
entityProps.forEach((name, value) => {
configEntries += new DescribeConfigsResponseData.DescribeConfigsResourceResult().setName(name.toString)
.setValue(value.toString).setConfigSource(ConfigSource.CLIENT_METRICS_CONFIG.id())
.setIsSensitive(false).setReadOnly(false).setSynonyms(List.empty.asJava)
})

new DescribeConfigsResponseData.DescribeConfigsResult()
.setErrorCode(Errors.NONE.code)
.setConfigs(configEntries.asJava)
val clientMetricsProps = configRepository.config(new ConfigResource(ConfigResource.Type.CLIENT_METRICS, resource.resourceName))
val clientMetricsConfig = ClientMetricsConfigs.fromProps(ClientMetricsConfigs.defaultConfigsMap(), clientMetricsProps)
createResponseConfig(allConfigs(clientMetricsConfig), createClientMetricsConfigEntry(clientMetricsConfig, clientMetricsProps, includeSynonyms, includeDocumentation))
}

case ConfigResource.Type.GROUP =>
Expand Down Expand Up @@ -185,8 +176,8 @@ class ConfigHelper(metadataCache: MetadataCache, config: KafkaConfig, configRepo
}
}

def createGroupConfigEntry(groupConfig: GroupConfig, groupProps: Properties, includeSynonyms: Boolean, includeDocumentation: Boolean)
(name: String, value: Any): DescribeConfigsResponseData.DescribeConfigsResourceResult = {
private def createGroupConfigEntry(groupConfig: GroupConfig, groupProps: Properties, includeSynonyms: Boolean, includeDocumentation: Boolean)
(name: String, value: Any): DescribeConfigsResponseData.DescribeConfigsResourceResult = {
val allNames = brokerSynonyms(name)
val configEntryType = GroupConfig.configType(name).toScala
val isSensitive = KafkaConfig.maybeSensitive(configEntryType)
Expand All @@ -210,6 +201,28 @@ class ConfigHelper(metadataCache: MetadataCache, config: KafkaConfig, configRepo
.setDocumentation(configDocumentation).setConfigType(dataType.id)
}

private def createClientMetricsConfigEntry(clientMetricsConfig: ClientMetricsConfigs, clientMetricsProps: Properties, includeSynonyms: Boolean, includeDocumentation: Boolean)
(name: String, value: Any): DescribeConfigsResponseData.DescribeConfigsResourceResult = {
val configEntryType = ClientMetricsConfigs.configType(name).asScala
val valueAsString = ConfigDef.convertToString(value, configEntryType.orNull)
val allSynonyms = {
if (!clientMetricsProps.containsKey(name)) {
Nil
} else {
new DescribeConfigsResponseData.DescribeConfigsSynonym().setName(name).setValue(valueAsString)
.setSource(ConfigSource.CLIENT_METRICS_CONFIG.id) :: Nil
}
}
val source = if (allSynonyms.isEmpty) ConfigSource.DEFAULT_CONFIG.id else allSynonyms.head.source
val synonyms = if (!includeSynonyms) List.empty else allSynonyms
val dataType = configResponseType(configEntryType)
val configDocumentation = if (includeDocumentation) clientMetricsConfig.documentationOf(name) else null
new DescribeConfigsResponseData.DescribeConfigsResourceResult()
.setName(name).setValue(valueAsString).setConfigSource(source)
.setIsSensitive(false).setReadOnly(false).setSynonyms(synonyms.asJava)
.setDocumentation(configDocumentation).setConfigType(dataType.id)
}

def createTopicConfigEntry(logConfig: LogConfig, topicProps: Properties, includeSynonyms: Boolean, includeDocumentation: Boolean)
(name: String, value: Any): DescribeConfigsResponseData.DescribeConfigsResourceResult = {
val configEntryType = LogConfig.configType(name).toScala
Expand Down
18 changes: 9 additions & 9 deletions core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -233,12 +232,14 @@ private void verifyClientMetricsConfigUpdate(List<String> alterOpts) throws Exce
try (Admin client = cluster.admin()) {
// Add config
Map<String, String> configs = new HashMap<>();
configs.put("metrics", "");
configs.put("metrics", "org.apache.kafka.producer.");
configs.put("interval.ms", "6000");
alterAndVerifyClientMetricsConfig(client, defaultClientMetricsName, configs, alterOpts);

// Delete config
deleteAndVerifyClientMetricsConfigValue(client, defaultClientMetricsName, configs.keySet(), alterOpts);
configs.put("metrics", "");
configs.put("interval.ms", "300000");
deleteAndVerifyClientMetricsConfigValue(client, defaultClientMetricsName, configs, alterOpts);

// Unknown config configured should fail
assertThrows(ExecutionException.class, () -> alterConfigWithAdmin(client, singletonMap("unknown.config", "20000"), alterOpts));
Expand Down Expand Up @@ -319,7 +320,7 @@ public void testUpdateInvalidBrokerConfigs() {
private void updateAndCheckInvalidBrokerConfig(Optional<String> brokerIdOrDefault) {
List<String> alterOpts = generateDefaultAlterOpts(cluster.bootstrapServers());
try (Admin client = cluster.admin()) {
alterConfigWithAdmin(client, brokerIdOrDefault, Collections.singletonMap("invalid", "2"), alterOpts);
alterConfigWithAdmin(client, brokerIdOrDefault, Map.of("invalid", "2"), alterOpts);

Stream<String> describeCommand = Stream.concat(
Stream.concat(
Expand All @@ -339,7 +340,7 @@ private void updateAndCheckInvalidBrokerConfig(Optional<String> brokerIdOrDefaul
public void testUpdateInvalidTopicConfigs() throws ExecutionException, InterruptedException {
List<String> alterOpts = asList("--bootstrap-server", cluster.bootstrapServers(), "--entity-type", "topics", "--alter");
try (Admin client = cluster.admin()) {
client.createTopics(Collections.singletonList(new NewTopic("test-config-topic", 1, (short) 1))).all().get();
client.createTopics(List.of(new NewTopic("test-config-topic", 1, (short) 1))).all().get();
assertInstanceOf(
InvalidConfigurationException.class,
assertThrows(
Expand Down Expand Up @@ -558,17 +559,16 @@ private void deleteAndVerifyGroupConfigValue(Admin client,

private void deleteAndVerifyClientMetricsConfigValue(Admin client,
String clientMetricsName,
Set<String> defaultConfigs,
Map<String, String> defaultConfigs,
List<String> alterOpts) throws Exception {
List<String> bootstrapOpts = quorumArgs().collect(Collectors.toList());
ConfigCommand.ConfigCommandOptions deleteOpts =
new ConfigCommand.ConfigCommandOptions(toArray(bootstrapOpts,
alterOpts,
asList("--delete-config", String.join(",", defaultConfigs))));
asList("--delete-config", String.join(",", defaultConfigs.keySet()))));
deleteOpts.checkArgs();
ConfigCommand.alterConfig(client, deleteOpts);
// There are no default configs returned for client metrics
verifyClientMetricsConfig(client, clientMetricsName, Collections.emptyMap());
verifyClientMetricsConfig(client, clientMetricsName, defaultConfigs);
}

private void verifyPerBrokerConfigValue(Admin client,
Expand Down
4 changes: 0 additions & 4 deletions core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,10 +971,6 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest {
val groupResource = new ConfigResource(ConfigResource.Type.GROUP, "none_group")
val groupResult = client.describeConfigs(Seq(groupResource).asJava).all().get().get(groupResource)
assertNotEquals(0, groupResult.entries().size())

val metricResource = new ConfigResource(ConfigResource.Type.CLIENT_METRICS, "none_metric")
val metricResult = client.describeConfigs(Seq(metricResource).asJava).all().get().get(metricResource)
assertEquals(0, metricResult.entries().size())
Comment on lines -975 to -977
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do need to remove this line? Is it because now default configs will always appear for unknown subscription? Will it add confusion when describing? Should we just add a note in describe that if subscription doesn not exist then default values will be displayed, and check for subscriptions list to find which all subscriptions are applied to the cluster.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes, that's what happens. Previously, if you displayed a non-existent resource, it didn't say "This doesn't exist", it just printed an empty list. Now, it prints the defaults. What ideally would happen is that it would say "This resource doesn't exist." but I think that's tricky to achieve. If you have ideas here, let me know. I'd prefer if the resource was flagged as an error in this case but the client metrics config manager in the broker is a tricky beast I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've dug more into this. The behaviour is the same for groups and client metrics. If you describe a non-existent configs resource, it returns an empty Properties() (this is in ConfigurationsImage.configProperties(ConfigResource). Then, the defaults are applied to this empty set which makes it look like the config resource exists.

I think it's worth making this work properly such that describing an existing resource works, and a non-existent resource fails neatly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree shall we do it as part of this PR or shall take it separtely? Whatever works with you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having taken a look at the code in more detail, I would prefer to do it as a follow-on PR. This PR would grow quite a bit doing both things at once.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected as well, thanks a lot @AndrewJSchofield for the fix and additional work.

}

@ParameterizedTest
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ class ControllerConfigurationValidatorTest {
@Test
def testValidClientMetricsConfig(): Unit = {
val config = new util.TreeMap[String, String]()
config.put(ClientMetricsConfigs.PUSH_INTERVAL_MS, "2000")
config.put(ClientMetricsConfigs.SUBSCRIPTION_METRICS, "org.apache.kafka.client.producer.partition.queue.,org.apache.kafka.client.producer.partition.latency")
config.put(ClientMetricsConfigs.CLIENT_MATCH_PATTERN, "client_instance_id=b69cc35a-7a54-4790-aa69-cc2bd4ee4538,client_id=1" +
config.put(ClientMetricsConfigs.INTERVAL_MS_CONFIG, "2000")
config.put(ClientMetricsConfigs.METRICS_CONFIG, "org.apache.kafka.client.producer.partition.queue.,org.apache.kafka.client.producer.partition.latency")
config.put(ClientMetricsConfigs.MATCH_CONFIG, "client_instance_id=b69cc35a-7a54-4790-aa69-cc2bd4ee4538,client_id=1" +
",client_software_name=apache-kafka-java,client_software_version=2.8.0-SNAPSHOT,client_source_address=127.0.0.1," +
"client_source_port=1234")
validator.validate(new ConfigResource(CLIENT_METRICS, "subscription-1"), config, emptyMap())
Expand All @@ -147,12 +147,12 @@ class ControllerConfigurationValidatorTest {
@Test
def testInvalidIntervalClientMetricsConfig(): Unit = {
val config = new util.TreeMap[String, String]()
config.put(ClientMetricsConfigs.PUSH_INTERVAL_MS, "10")
config.put(ClientMetricsConfigs.INTERVAL_MS_CONFIG, "10")
assertEquals("Invalid value 10 for interval.ms, interval must be between 100 and 3600000 (1 hour)",
assertThrows(classOf[InvalidRequestException], () => validator.validate(
new ConfigResource(CLIENT_METRICS, "subscription-1"), config, emptyMap())). getMessage)

config.put(ClientMetricsConfigs.PUSH_INTERVAL_MS, "3600001")
config.put(ClientMetricsConfigs.INTERVAL_MS_CONFIG, "3600001")
assertEquals("Invalid value 3600001 for interval.ms, interval must be between 100 and 3600000 (1 hour)",
assertThrows(classOf[InvalidRequestException], () => validator.validate(
new ConfigResource(CLIENT_METRICS, "subscription-1"), config, emptyMap())). getMessage)
Expand All @@ -170,7 +170,7 @@ class ControllerConfigurationValidatorTest {
@Test
def testInvalidMatchClientMetricsConfig(): Unit = {
val config = new util.TreeMap[String, String]()
config.put(ClientMetricsConfigs.CLIENT_MATCH_PATTERN, "10")
config.put(ClientMetricsConfigs.MATCH_CONFIG, "10")
assertEquals("Illegal client matching pattern: 10",
assertThrows(classOf[InvalidConfigurationException], () => validator.validate(
new ConfigResource(CLIENT_METRICS, "subscription-1"), config, emptyMap())). getMessage)
Expand Down
12 changes: 6 additions & 6 deletions server/src/main/java/org/apache/kafka/server/ClientMetricsManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,9 @@ public void close() throws Exception {
}

private void updateClientSubscription(String subscriptionName, ClientMetricsConfigs configs) {
List<String> metrics = configs.getList(ClientMetricsConfigs.SUBSCRIPTION_METRICS);
int pushInterval = configs.getInt(ClientMetricsConfigs.PUSH_INTERVAL_MS);
List<String> clientMatchPattern = configs.getList(ClientMetricsConfigs.CLIENT_MATCH_PATTERN);
List<String> metrics = configs.getList(ClientMetricsConfigs.METRICS_CONFIG);
int pushInterval = configs.getInt(ClientMetricsConfigs.INTERVAL_MS_CONFIG);
List<String> clientMatchPattern = configs.getList(ClientMetricsConfigs.MATCH_CONFIG);

SubscriptionInfo newSubscription =
new SubscriptionInfo(subscriptionName, metrics, pushInterval,
Expand Down Expand Up @@ -329,7 +329,7 @@ private ClientMetricsInstance createClientInstanceAndUpdateCache(Uuid clientInst

private ClientMetricsInstance createClientInstance(Uuid clientInstanceId, ClientMetricsInstanceMetadata instanceMetadata) {

int pushIntervalMs = ClientMetricsConfigs.DEFAULT_INTERVAL_MS;
int pushIntervalMs = ClientMetricsConfigs.INTERVAL_MS_DEFAULT;
// Keep a set of metrics to avoid duplicates in case of overlapping subscriptions.
Set<String> subscribedMetrics = new HashSet<>();
boolean allMetricsSubscribed = false;
Expand All @@ -338,7 +338,7 @@ private ClientMetricsInstance createClientInstance(Uuid clientInstanceId, Client
for (SubscriptionInfo info : subscriptionMap.values()) {
if (instanceMetadata.isMatch(info.matchPattern())) {
allMetricsSubscribed = allMetricsSubscribed || info.metrics().contains(
ClientMetricsConfigs.ALL_SUBSCRIBED_METRICS_CONFIG);
ClientMetricsConfigs.ALL_SUBSCRIBED_METRICS);
subscribedMetrics.addAll(info.metrics());
pushIntervalMs = Math.min(pushIntervalMs, info.intervalMs());
}
Expand All @@ -351,7 +351,7 @@ private ClientMetricsInstance createClientInstance(Uuid clientInstanceId, Client
if (allMetricsSubscribed) {
// Only add an * to indicate that all metrics are subscribed.
subscribedMetrics.clear();
subscribedMetrics.add(ClientMetricsConfigs.ALL_SUBSCRIBED_METRICS_CONFIG);
subscribedMetrics.add(ClientMetricsConfigs.ALL_SUBSCRIBED_METRICS);
}

int subscriptionId = computeSubscriptionId(subscribedMetrics, pushIntervalMs, clientInstanceId);
Expand Down
Loading
Loading