mjsax commented on code in PR #14575: URL: https://github.com/apache/kafka/pull/14575#discussion_r1374007941
########## clients/src/main/java/org/apache/kafka/common/telemetry/internals/MetricKey.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.telemetry.internals; + +import org.apache.kafka.common.MetricName; + +import java.util.Collections; +import java.util.Map; +import java.util.Objects; + +/** + * Value object that contains the name and tags for a Metric. + */ +public class MetricKey implements MetricKeyable { + + private final String name; + private final Map<String, String> tags; + + /** + * Create a {@code MetricKey} + * + * @param name metric name. This should be the telemetry metric name of the metric (the final name + * under which this metric is emitted). + */ + public MetricKey(String name) { + this(name, null); + } + + /** + * Create a {@code MetricKey} + * + * @param name metric name. This should be the .converted. name of the metric (the final name + * under which this metric is emitted). + * @param tags mapping of tag keys to values. + */ + public MetricKey(String name, Map<String, String> tags) { + this.name = Objects.requireNonNull(name); + this.tags = tags != null ? Collections.unmodifiableMap(tags) : Collections.emptyMap(); + } + + public MetricKey(MetricName metricName) { + this(metricName.name(), metricName.tags()); + } + + @Override + public MetricKey key() { + return this; + } + + public String getName() { Review Comment: nit: we don't use `get` prefix as a convention; should be rename to `name()` ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryPayload.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.annotation.InterfaceStability; + +import java.nio.ByteBuffer; + [email protected] +public interface ClientTelemetryPayload { Review Comment: Should we add a class level JavaDoc describing it? It's a public interface, so seems important? ########## clients/src/main/java/org/apache/kafka/common/telemetry/ClientTelemetry.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.telemetry; + +import org.apache.kafka.common.metrics.MetricsContext; +import org.apache.kafka.common.metrics.MetricsReporter; +import org.apache.kafka.common.annotation.InterfaceStability; + +import java.util.Optional; + +/** + * A {@link MetricsReporter} may implement this interface to indicate support for collecting client + * telemetry on the client or server side. + */ [email protected] +public interface ClientTelemetry { + + /** + * Implemented by the client {@link MetricsReporter} to provide a {@link ClientTelemetrySender} + * instance. + * <p> + * This instance may be cached by the client. + * <p> + * This method must always be called after the initial call to + * {@link MetricsReporter#contextChange(MetricsContext)} on the {@link MetricsReporter} + * implementing this interface. Review Comment: Well, but it seems to be an comment for somebody writing broker code, not for the user implementing this interface? Thus, it seems it should not be part of the public JavaDocs -- user implementing a broker plugin won't care, because they don't make these calls? ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryPayload.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.annotation.InterfaceStability; + +import java.nio.ByteBuffer; + [email protected] +public interface ClientTelemetryPayload { + + /** + * @return Client's instance id. Review Comment: Even if redundant, we should add a sentence for the JavaDoc -- public API JavaDocs will be rendered an people will ready it, and it's weird if there is no description but only a `@return` tag. ``` Returns the client's instance id. ``` Similar below for other methods. ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetry.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.metrics.MetricsContext; +import org.apache.kafka.common.metrics.MetricsReporter; +import org.apache.kafka.common.annotation.InterfaceStability; + +/** + * A {@link MetricsReporter} may implement this interface to indicate support for collecting client + * telemetry on the server side. + */ [email protected] +public interface ClientTelemetry { + + /** + * Implemented by the broker {@link MetricsReporter} to provide a {@link ClientTelemetryReceiver} Review Comment: > Implemented by the broker {@link MetricsReporter} Sound weird -- I would not use the term "Implemented" for a method, because it's clear that if one implement the interface they must implement all methods... -- Also "by the broker" sound like as if the was a default implementation what is not the case. In the end, we should only describe what this method does (might also be good to clarify if it's expected to return a new object on each call; guess we can omit it if there is no restriction and a cached or singleton object could be returned, too): ``` Creates a {@link ClientTelemetryReceiver} instance. ``` ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryReceiver.java: ########## @@ -0,0 +1,37 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.annotation.InterfaceStability; +import org.apache.kafka.server.authorizer.AuthorizableRequestContext; + [email protected] +public interface ClientTelemetryReceiver { + /** + * Called by the broker when a client reports telemetry metrics. The associated request context + * can be used by the metrics plugin to retrieve additional client information such as client ids + * or endpoints. + * <p>s Review Comment: remove `s` ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryReceiver.java: ########## @@ -0,0 +1,37 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.annotation.InterfaceStability; +import org.apache.kafka.server.authorizer.AuthorizableRequestContext; + [email protected] +public interface ClientTelemetryReceiver { Review Comment: Add class JavaDocs ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryPayload.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.annotation.InterfaceStability; + +import java.nio.ByteBuffer; + [email protected] +public interface ClientTelemetryPayload { + + /** + * @return Client's instance id. + */ + Uuid clientInstanceId(); + + /** + * Indicates whether client is terminating, e.g., the last metrics push from this client instance. + * <p> + *To avoid the receiving broker’s metrics rate-limiter discarding this out-of-profile push, the Review Comment: nit: missing space ########## clients/src/main/java/org/apache/kafka/server/telemetry/ClientTelemetryPayload.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.server.telemetry; + +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.annotation.InterfaceStability; + +import java.nio.ByteBuffer; + [email protected] +public interface ClientTelemetryPayload { + + /** + * @return Client's instance id. + */ + Uuid clientInstanceId(); + + /** + * Indicates whether client is terminating, e.g., the last metrics push from this client instance. + * <p> + *To avoid the receiving broker’s metrics rate-limiter discarding this out-of-profile push, the Review Comment: This comment seems to explain protocol details -- why would an end-user care? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
