KAFKA-19882 Cleaning client level metric tags #20883
Conversation
bbejeck
left a comment
bbejeck
left a comment
There was a problem hiding this comment.
@gensericghiro thanks for the PR - I've made an initial pass
| mkEntry("client-id", CLIENT_ID) | ||
| ) | ||
| ); | ||
| assertThat(metrics.metric(name), notNullValue()); |
There was a problem hiding this comment.
Why just not null vs. asserting the name is equal?
There was a problem hiding this comment.
Agreed, but I was being consistent with what's here. I can change both
There was a problem hiding this comment.
Let's change the other code, too. It's a little weird to not check the name (or is there some non-deterministic naming, like a "thread name" which could mess with us?)
| mkEntry("additional-tag", "additional-value") | ||
| ) | ||
| ); | ||
| assertThat(metrics.metric(name), notNullValue()); |
| final RecordingLevel recordingLevel, | ||
| final T value) { | ||
| final MetricName metricName = metrics.metricName(name, CLIENT_LEVEL_GROUP, description, | ||
| clientLevelTagMap(additionalTags)); |
There was a problem hiding this comment.
nit: Formatting. Why the line break? Should just go to above line?
There was a problem hiding this comment.
Do we not enforce a max 100 width on columns? It makes the code hard to read otherwise
There was a problem hiding this comment.
No that I know of (and 100 would be very tight). As long as checkstyle does not complain, we are good.
The other point is: if we really have too many parameter, the current formatting with a single line break is also very hard to read, and if we thing the line gets to long, we should go with a single-parameter per line formatting:
final MetricName metricName = metrics.metricName(
name,
CLIENT_LEVEL_GROUP,
description,
clientLevelTagMap(additionalTags)
);
| final RecordingLevel recordingLevel, | ||
| final Gauge<T> valueProvider) { | ||
| final MetricName metricName = metrics.metricName(name, CLIENT_LEVEL_GROUP, description, | ||
| clientLevelTagMap(additionalTags)); |
...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
Show resolved
Hide resolved
| final RecordingLevel recordingLevel = RecordingLevel.INFO; | ||
| setupGetNewSensorTest(metrics); | ||
| final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, PROCESS_ID, APPLICATION_ID, | ||
| final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, |
There was a problem hiding this comment.
nit: formatting. Let's move time parameter into this line, too, for some cleanup (same below)
| mkEntry("client-id", CLIENT_ID) | ||
| ) | ||
| ); | ||
| assertThat(metrics.metric(name), notNullValue()); |
There was a problem hiding this comment.
Same question as Bill asks further below
There was a problem hiding this comment.
Agreed, but I was being consistent with what's here. I can change both
| mkEntry("additional-tag", "additional-value") | ||
| ) | ||
| ); | ||
| assertThat(metrics.metric(name), notNullValue()); |
| public void shouldRemoveClientLevelMetricsAndSensors() { | ||
| final Metrics metrics = mock(Metrics.class); | ||
| final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, PROCESS_ID, APPLICATION_ID, | ||
| final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, |
There was a problem hiding this comment.
Nit: more formatting cleanup in this file, too :)
|
@gensericghiro Please fix the check style failure in the build. |
4 participants
Summary
When working on
KIP-1091,
we mistakenly applied the
process-idtag to all client-level metrics,rather than just the
client-state,thread-state, andrecording-levelmetrics as specified in the KIP. This issue came tolight while working on KIP-1221, which aimed to add the
application-idas a tag to the
client-statemetric introduced by KIP-1091. This PRremoves these tags from all metrics by default, and adds them to only
the
client-state(application-id + process-id) and therecording-level(process-id only)Tests
Unit tests in
ClientMetricsTest.javaandStreamsMetricsImplTest.java