KAFKA-8934: Introduce instance-level metrics for streams applications by cadonna · Pull Request #7416 · apache/kafka

cadonna · 2019-09-30T09:08:11Z

Moves StreamsMetricsImpl from StreamThread to KafkaStreams
Adds instance-level metrics as specified in KIP-444, i.e.:
-- version
-- commit-id
-- application-id
-- topology-description
-- state

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

cadonna

Call for review: @guozhangwang @vvcephei @bbejeck

Sorry for the large PR.

cadonna · 2019-09-30T09:09:25Z

Here I add the instance-level metrics.

cadonna · 2019-09-30T09:10:11Z

StreamsMetricsImpl is now created at client level and not on thread level anymore.

cadonna · 2019-09-30T09:11:40Z

This is not needed anymore since the RocksDBMetricsRecordingTrigger is contained in StreamsMetricsImpl.

cadonna · 2019-09-30T09:14:50Z

The threadId needs to be passed to those methods since StreamsMetricsImpl was moved to KafkaStreams and cannot contain the thread ID anymore.

cadonna · 2019-09-30T09:16:49Z

Verifications of state, topology description, and application ID metrics.

cadonna · 2019-09-30T09:17:35Z

Verification of state metric.

cadonna · 2019-09-30T19:51:03Z

JDK 11/Scala 2.13 and JDK 8/Scala 2.11 failed due to OOM.

In JDK 11/Scala 2.12, the following test failed:

kafka.admin.LeaderElectionCommandTest.testPathToJsonFile

Retest this, please

cadonna · 2019-10-01T06:44:09Z

In JDK 8/Scala 2.11, the following test failed:

kafka.api.SaslOAuthBearerSslEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl

Retest this, please

bbejeck

Thanks for the Metrics PR @cadonna. I've made a pass, overall looks good, I just have minor comments.

bbejeck · 2019-10-01T15:35:24Z

nit: can we log the stacktrace here? IMHO it's better for debugging.

+1

FYI @cadonna , all you have to do is:

Suggested change

log.warn("Error while loading kafka-streams-version.properties: {}", exception.getMessage());

log.warn("Error while loading kafka-streams-version.properties", exception);

bbejeck · 2019-10-01T15:56:54Z

We seem to use StreamsMetricsImpl in several places, would it be better to accept a type StreamsMetrics and cast it internally to StreamsMetricsImpl? Same comment applies to here and elsewhere. I might be missing some context here though.

StreamsMetrics is our public interface to interact with metrics. StreamsMetricsImpl is our internal implementation that also has methods to create our built-in metrics. As far as I know, we pass StreamsMetricsImpl only to internal objects where we need the methods for built-in metrics. A Streams user would only see the StreamsMetrics interface. Said that, I think it is fine as it is.

My two cents: it's better not to cast. If we need the "internal interface" (i.e., Impl) here, then we should simply declare that fact in the parameter type. Then, the calling code would always know it has to pass an Impl, and we wouldn't ever get surprise ClassCastExceptions in tests or runtime code.

bbejeck · 2019-10-01T16:08:12Z

nit: break args on a separate line

bbejeck · 2019-10-01T16:15:18Z

super nit: Maybe add a brief comment that this is used for metrics like app-id etc. I needed to do a search through the code to see why it was necessary.

I prefer to rename the class to ImmutableMetricValue.

bbejeck · 2019-10-01T16:35:53Z

super nit: make string task a public final variable.

bbejeck · 2019-10-01T16:47:20Z

Also fixed in PR #7429.

bbejeck · 2019-10-01T16:47:31Z

bbejeck · 2019-10-01T16:48:25Z

nit: same here for string literals and below, I won't repeat this comment anymore.

I will tackle this in the PR for the store-level metrics.

Is multi-line here for checkstyle to be happy? Honestly I like the previous one as it is easier to compare multi-lines in case there's any parameter mis-set, would be great if we can get back to oneliner.

It is not checkstyle, but our coding style guidelines. I see what you mean. I guess we should refactor this anyways to hide most of those parameters behind a method. Looking at the other refactorings, this should become something along the lines of putLatencySensor(threadId, taskId, metricsScope, storeName) which would fit again on one line.

bbejeck · 2019-10-01T16:50:49Z

nit: parameter on separate line

bbejeck · 2019-10-01T16:51:41Z

nit: parameters on separate line

vvcephei · 2019-10-02T04:13:36Z

Just going out on a limb here... should we also log the topology description here?

I thought, we already do, but I couldn't find any output in a log file that I have locally. I would be in favour of logging the topology because it helps us during on-call. On the other hand, it may pollute the logs. Anyways, could we postpone this discussion to after the release?

vvcephei

Just a couple of quick comments.

vvcephei · 2019-10-02T04:16:30Z

+1

FYI @cadonna , all you have to do is:

Suggested change

log.warn("Error while loading kafka-streams-version.properties: {}", exception.getMessage());

log.warn("Error while loading kafka-streams-version.properties", exception);

vvcephei · 2019-10-02T04:18:02Z

This seems to be more like "InstanceMetrics" than "client" metrics, where "client" usually means a Producer or Consumer. In Streams, we sometimes conflate "client" with "thread" because each StreamThread has exactly one Consumer.

In KIP-444, "client" is used instead of "instance". Also the tag for instance-level is "client-id" whereas for thread it will be "thread-id" with PR #7429. I agree that clients is somehow overloaded, but since Streams is also a client (that uses other clients) it seems consistent to me.

vvcephei · 2019-10-02T19:45:24Z

Is this always right? It would set the threadId as the name of the thread that constructed the sensor, but is this always the StreamThread? Even if it is today, it wouldn't be outrageous to think in the future that another thread might build a task and then pass it to the thread to execute. Such a refactoring would break this logic, but it would be very subtle.

In contrast, the old code here explicitly passed the thread name via the context, so it would never be set incorrectly just by changing the way that the code is invoked. Perhaps we could preserve this property, for example by passing the thread name as a method argument.

Just checked the code path, passing the thread name via GlobalStreamThread / StreamThread -> StreamTask -> ProcessorContextImpl maybe fine.

@vvcephei, you are right. This is not general enough. On thread-level -- for instance -- sensors are created by the main thread and then used in the stream thread. Here, instead of passing it through InternalProcessorContext, I would pass it as a separate parameter to lateRecordDropSensor(). According to KIP-444 late-record-drop will be superseded with dropped-records and lateRecordDropSensor() will most probably disappear. Actually, since it is currently not a problem at all because the calling thread is the stream thread, I would leave it as it is.

Ok, sounds fine to me, as long as we're aware of the risk.

vvcephei · 2019-10-02T19:47:52Z

My two cents: it's better not to cast. If we need the "internal interface" (i.e., Impl) here, then we should simply declare that fact in the parameter type. Then, the calling code would always know it has to pass an Impl, and we wouldn't ever get surprise ClassCastExceptions in tests or runtime code.

guozhangwang

Made a pass. One meta comment: lots of LOCs are actually extending the single line to multi-lines because of the insertion of thread-id, if it can be avoided I'd prefer the old way since if there are multi sensors to be added together it's actually easier to review.

guozhangwang · 2019-10-02T21:55:11Z

After the other PR is merged, this part can be leveraged / rebased, just a reminder :)

guozhangwang · 2019-10-02T22:05:01Z

Just checked the code path, passing the thread name via GlobalStreamThread / StreamThread -> StreamTask -> ProcessorContextImpl maybe fine.

guozhangwang · 2019-10-02T22:08:22Z

Can we rely on ImmutableValue from o.a.k.common?

ImmutableMetricValue is basically a copy of ImmutableValue. ImmutableValue in o.a.k.common is package private and within AppInfoParser. Additionally, I needed to add hashCode() and equals() for testing purposes. What we can do after the release is refactoring the ImmutableValue in common and use it in streams.

guozhangwang · 2019-10-02T22:12:32Z

Is multi-line here for checkstyle to be happy? Honestly I like the previous one as it is easier to compare multi-lines in case there's any parameter mis-set, would be great if we can get back to oneliner.

guozhangwang · 2019-10-02T22:13:22Z

Ditto here.

See my comment above.

FYI: We have have a bug here. We use fetch instead of get and users have already asked a question about this on Stackoverflow. KAFKA-8906 documents this.

guozhangwang · 2019-10-02T22:16:59Z

Ditto here, I think besides the threadId having other paired values in oneline is better to read / review.

This is refactored in PR #7429.

guozhangwang · 2019-10-02T22:21:58Z

In KIP-444 the one below would be stream-thread-metrics.

This is refactored in the thread-level refactoring PR that I could not yet open because it is based on PR #7429.

guozhangwang · 2019-10-02T22:32:02Z

Learned new ways of functional interfaces :)

bbejeck · 2019-10-03T17:24:14Z

Java 11/2.12 failed and Java 8 failed; results already cleared out

retest this please

bbejeck · 2019-10-03T19:29:58Z

@cadonna failures seem related Execution failed for task ':streams:compileTestJava'.

guozhangwang · 2019-10-03T20:01:28Z

The new commit lgtm. Leave to @bbejeck to merge.

Let's do this one and then #7429

- Moves `StreamsMetricsImpl` from `StreamThread` to `KafkaStreams` - Adds instance-level metrics as specified in KIP-444, i.e.: -- version -- commit-id -- application-id -- topology-description -- state

Also adds comments to tests that possibly fail when started from an IDE due to the missing version file on the classpath.

Fixes compile error introduced by merging apache#5527

cadonna · 2019-10-04T17:05:42Z

In JDK 11/Scala 2.12, the following test failed:

kafka.api.GroupAuthorizerIntegrationTest.testConsumeWithoutTopicDescribeAccess

In JDK 11/Scala 2.13, the following test failed:

org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector

In JDK 8/Scala 2.11, the following test failed:

kafka.api.SaslGssapiSslEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl

Retest this, please

guozhangwang · 2019-10-05T00:06:37Z

Jenkins failures are irrelevant.

guozhangwang · 2019-10-05T00:07:38Z

Merged to trunk.

cadonna commented Sep 30, 2019

View reviewed changes

bbejeck added the streams label Oct 1, 2019

bbejeck reviewed Oct 1, 2019

View reviewed changes

vvcephei reviewed Oct 2, 2019

View reviewed changes

cadonna force-pushed the add_client_level_metrics branch from 73ab316 to bf3ec27 Compare October 2, 2019 10:21

vvcephei reviewed Oct 2, 2019

View reviewed changes

guozhangwang reviewed Oct 2, 2019

View reviewed changes

cadonna force-pushed the add_client_level_metrics branch from bf3ec27 to 64a5fba Compare October 3, 2019 20:06

cadonna added 5 commits October 4, 2019 10:18

KAFKA-8934: Introduce instance-level metrics for streams applications

7ec0ca0

- Moves `StreamsMetricsImpl` from `StreamThread` to `KafkaStreams` - Adds instance-level metrics as specified in KIP-444, i.e.: -- version -- commit-id -- application-id -- topology-description -- state

Correct name of version file

8bc5eae

Also adds comments to tests that possibly fail when started from an IDE due to the missing version file on the classpath.

Include feedback

051eccb

Fix compile error

bb91d19

Fix compile error

ab3c043

Fixes compile error introduced by merging apache#5527

cadonna force-pushed the add_client_level_metrics branch from ece0eda to ab3c043 Compare October 4, 2019 08:23

guozhangwang merged commit 52007e8 into apache:trunk Oct 5, 2019

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

	log.warn("Error while loading kafka-streams-version.properties: {}", exception.getMessage());
	log.warn("Error while loading kafka-streams-version.properties", exception);

Conversation

cadonna commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna commented Sep 30, 2019

Uh oh!

cadonna commented Oct 1, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

cadonna commented Sep 30, 2019 •

edited

Loading