Allow to bind KafkaClientMetrics for each decaton consumer #132

mauhiz · 2021-11-22T01:51:52Z

Motivation

micrometer 1.4+ has deprecated the JMX-based based metrics gathering ( KafkaConsumerMetrics ) in favour of converting Consumer exposed Metrics. See this PR.
decaton metrics don't cover some of the generic consumer metrics like records lag

Changes

Always bind KafkaClientMetrics, tagged with the right subscription id.
This should not be a breaking change since there is no reasonable way to have already bound KafkaClientMetrics for these consumers elsewhere.
Note : not doing this for KafkaProducerSupplier which is overridable / customizable.

(I'd like advice on how to deal with the test failure - I don't understand why we have to manage micrometer registry cleanup manually)

kawamuray · 2021-11-22T09:38:04Z

Thanks for PR!

I think one of the downsides of enabling KafkaClientMetrics by default is it would explode number of metrics in registry provided by decaton?
Decaton provides its own registry to let users easy control what to do with it, like exposing directly, or combining it with their own registry to mix the resulting metrics set.
While I think most of users will need kafka client's metrics in addition to decaton's metrics, I thought most of them using metrics.reporters to bypass decaton layers and inject client directly their own reporter, which sounds more suitable way than decaton to take care of it, but micormeter apparently doesn't provide out-of-the-box support for using metrics.reporters?
In case, I think supporting KafkaClientMetrics in decaton is good, but would it be better to expose ConsumerSupplier interface instead? or at least a flag to enable (disable) KafkaClientMetrics binding is needed IMO.

(I'd like advice on how to deal with the test failure - I don't understand why we have to manage micrometer registry cleanup manually)

That's because each decaton subscription creates metrics with its own label sets, so unless we clean these metrics up at the end of subscription's lifetime, there will be a garbage created which will keep accumulating. It wouldn't become a significant problem in most of decaton use-cases where decaton subscription dies along with application instance, but still should be cleaned up as possible as we can as a manner.

mauhiz · 2021-11-24T01:06:02Z

is it would explode number of metrics in registry provided by decaton?

yes, assuming people don't already monitor their Kafka client via JMX adapted (namely, micrometer's deprecated KafkaConsumerMetrics ).

micrometer apparently doesn't provide out-of-the-box support for using metrics.reporters

If I understand what you mean correctly, isn't that exactly the KafkaClientMetrics this PR is introducing?

expose ConsumerSupplier

My first thought, but it was not convenient since it doesn't have the subscriptionId (and not sure it should).

flag to enable/disable KafkaClientMetrics

Sounds good. Will add this flag.

clean these metrics up at the end of subscription's lifetime

I see, if we go the way I suggest in this PR I will fix to cleanup the metrics
What do other maintainers think?

mauhiz · 2021-11-24T02:13:42Z

I couldn't find a way to cleanup KafkaClientMetrics on subscription close. So, making it a more free option by exposing a builder for it, and leaving the binding up to the user.

kawamuray · 2021-11-25T03:40:28Z

If I understand what you mean correctly, isn't that exactly the KafkaClientMetrics this PR is introducing?

No, they favored the way using #metrics() method of Consumer to obtain metrics collection and wrap it over. What I said is to use metric.reporters property, which you specify the class name that implements MetricsReporter interface, so you can give it through properties you give to SubscriptionBuilder#consumerConfig().

kawamuray · 2021-11-25T03:42:57Z

I couldn't find a way to cleanup KafkaClientMetrics on subscription close.

Indeed with the current version, but looks like it has added in latest?

https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/kafka/KafkaMetrics.java#L295

mauhiz · 2021-11-25T07:27:18Z

metric.reporters property

Okay, I didn't know there was such a thing. I don't know why would they use or not use it :D

added in latest

a-ha, nice. Let me try to integrate it.

mauhiz · 2021-11-25T08:41:33Z

PTAL.
Note that these new metrics are prefixed with decaton. but that is fine, since they have an extra dimension (subscriptionId) they are not mergeable with generic consumer metrics

mauhiz · 2021-11-26T05:30:36Z

wow, many tests started timing out... why..

kawamuray · 2021-11-29T09:22:50Z

Just guessing, by increased number of metrics ...? We might need profiling to be sure.

mauhiz · 2021-12-01T05:40:18Z

It turns out that calling properties in TestUtils would override the properties defined right before in builderConfigurer.accept(builder).

I added commits to

avoid overwriting properties in SubscriptionBuilder - I can't see any good reason to do it
add timeouts of 20s in some places of the tests that were blocking forever, for easier debugging

kawamuray · 2021-12-03T10:19:13Z

processor/src/main/java/com/linecorp/decaton/processor/metrics/Metrics.java

+
+        public void bindClientMetrics(Consumer<String, byte[]> consumer) {
+            if (kafkaClientMetrics != null) {
+                closeClientMetrics();


In which case would this be effective?

Ah, none. Indeed SubscriptionMetrics are only instantiated in ProcessorSubscription ctor. Removing

kawamuray · 2021-12-03T10:25:37Z

processor/src/main/java/com/linecorp/decaton/processor/runtime/SubscriptionBuilder.java

@@ -59,8 +59,8 @@
        presetRetryProducerConfig.put(ProducerConfig.LINGER_MS_CONFIG, "100");
    }

-    @Setter(AccessLevel.NONE)
-    private ProcessorProperties.Builder<ProcessorProperties> propertiesBuilder;
+    private final ProcessorProperties.Builder<ProcessorProperties> propertiesBuilder =


avoid overwriting properties in SubscriptionBuilder - I can't see any good reason to do it

The intention is to provide a way to "clear" (reset) something set previously. So basically these methods taking collection of items replaces existing set rather than adding.
To keep this behavior consistent to other methods like consumerConfig and backward, I think we shouldn't change this.
I'm happy to add another kind of method like addProperties() (addProperty()) which supports it and being more explicit tho.

I don't think that's what one would expect of a builder, but happy to oblige with your design constraints

kawamuray · 2021-12-03T10:30:26Z

testing/src/main/java/com/linecorp/decaton/testing/TestUtils.java

@@ -62,6 +72,7 @@ private static Properties defaultProducerProps(String bootstrapServers) {
    }

    public static final String DEFAULT_GROUP_ID = "test-group";
+    public static final Duration DEFINITELY_TOO_SLOW = Duration.ofSeconds(20);


add timeouts of 20s in some places of the tests that were blocking forever, for easier debugging

curious why backtrace provided when test itself times out (@Test(timeout = xxx)) isn't sufficient to know at where it stuck?

because it was not exactly stuck. Producer and Consumer were running and doing nothing.

…lder" This reverts commit aca08e0.

…e builder

kawamuray

LGTM

kawamuray · 2021-12-07T07:32:16Z

testing/src/main/java/com/linecorp/decaton/testing/TestUtils.java

-                Thread.currentThread().interrupt();
-                throw new RuntimeException(e);
-            }
+            LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(100));


What's the intention of this change? Seems like waiting just 100ms here isn't an actual problem?

Just avoid having to deal with InterruptedException. No strong preference anyways

ocadaruma

Thank you!

LGTM except kawamuray's point.

mauhiz · 2021-12-09T09:46:47Z

Thanks for the reviews! Is the next release already scheduled?

kawamuray · 2021-12-13T02:37:32Z

It should be in next few weeks, but if you're in hurry, I can make a release now.

mauhiz · 2021-12-13T04:39:43Z

I'm not exactly in a hurry but finer-grained releases usually make happier users!

mauhiz force-pushed the feature-poc-KafkaClientMetrics branch from 8741661 to cefcab2 Compare November 24, 2021 02:10

mauhiz changed the title ~~Bind micrometer KafkaClientMetrics to each decaton consumer~~ Allow to bind KafkaClientMetrics for each decaton consumer Nov 24, 2021

Bind KafkaClientMetrics on demand

7036afb

mauhiz force-pushed the feature-poc-KafkaClientMetrics branch from cefcab2 to 7036afb Compare November 25, 2021 08:40

mauhiz added 3 commits December 1, 2021 14:22

Add reasonable timeouts on each step to help debug timeout issues

3fc39f1

Null kafkaClientMetrics to avoid closing twice

1340444

Do not erase previous state when adding properties to the builder

aca08e0

kawamuray suggested changes Dec 3, 2021

View reviewed changes

mauhiz added 3 commits December 6, 2021 12:56

(fixup) no need to closeClientMetrics on bindClientMetrics

92c4a5c

Revert "Do not erase previous state when adding properties to the bui…

273b1a1

…lder" This reverts commit aca08e0.

Introduce addProperties to add PropertySuppliers without resetting th…

df09b3c

…e builder

mauhiz requested a review from kawamuray December 6, 2021 04:31

kawamuray approved these changes Dec 7, 2021

View reviewed changes

ocadaruma approved these changes Dec 7, 2021

View reviewed changes

kawamuray merged commit 75d8840 into line:master Dec 8, 2021

mauhiz deleted the feature-poc-KafkaClientMetrics branch December 8, 2021 08:37

kawamuray added the new feature Add a new feature label Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to bind KafkaClientMetrics for each decaton consumer #132

Allow to bind KafkaClientMetrics for each decaton consumer #132

mauhiz commented Nov 22, 2021 •

edited

Loading

kawamuray commented Nov 22, 2021

mauhiz commented Nov 24, 2021

mauhiz commented Nov 24, 2021

kawamuray commented Nov 25, 2021

kawamuray commented Nov 25, 2021

mauhiz commented Nov 25, 2021

mauhiz commented Nov 25, 2021

mauhiz commented Nov 26, 2021

kawamuray commented Nov 29, 2021

mauhiz commented Dec 1, 2021

kawamuray Dec 3, 2021

mauhiz Dec 6, 2021

kawamuray Dec 3, 2021

mauhiz Dec 6, 2021

kawamuray Dec 3, 2021

mauhiz Dec 6, 2021

kawamuray left a comment

kawamuray Dec 7, 2021

mauhiz Dec 7, 2021

ocadaruma left a comment

mauhiz commented Dec 9, 2021

kawamuray commented Dec 13, 2021

mauhiz commented Dec 13, 2021

Allow to bind KafkaClientMetrics for each decaton consumer #132

Allow to bind KafkaClientMetrics for each decaton consumer #132

Conversation

mauhiz commented Nov 22, 2021 • edited Loading

Motivation

Changes

kawamuray commented Nov 22, 2021

mauhiz commented Nov 24, 2021

mauhiz commented Nov 24, 2021

kawamuray commented Nov 25, 2021

kawamuray commented Nov 25, 2021

mauhiz commented Nov 25, 2021

mauhiz commented Nov 25, 2021

mauhiz commented Nov 26, 2021

kawamuray commented Nov 29, 2021

mauhiz commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawamuray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocadaruma left a comment

Choose a reason for hiding this comment

mauhiz commented Dec 9, 2021

kawamuray commented Dec 13, 2021

mauhiz commented Dec 13, 2021

mauhiz commented Nov 22, 2021 •

edited

Loading