KAFKA-5900: Add task metrics common to both sink and source tasks #3911

rhauch · 2017-09-19T22:11:17Z

Added metrics that are common to both sink and source tasks.

Marked as "WIP" since this PR is built upon #3864, and will need to be rebased once that has been merged into trunk. However, I would still appreciate initial reviews since this PR is largely additive.

rhauch · 2017-09-19T22:15:44Z

@kkonstantine can you review this after you've given #3864 a once-over?

kkonstantine

I gave it a first pass, skipping the parts that I reviewed already in KAFKA-5899.

It'll be much easier to do another pass after the above is merged. Focused on code and just glanced over tests.

My main question is around the correctness and the efficiency of StateTracker. Also any mention to thread-safety should be included to its javadoc.

kkonstantine · 2017-09-20T11:59:10Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+     * @return the Frequencies instance; never null
+     * @throws IllegalArgumentException if both {@code falseMetricName} and {@code trueMetricName} are null
+     */
+    public static Frequencies forBooleanValues(MetricName falseMetricName, MetricName trueMetricName) {


Would you agree with public static methods following member field definition and constructor definition?

I think public static methods should appear before all (non-static) members. In this case, it is a factory method, so it is as important as constructors. IMO, static members should not really be interspersed with non-static members.

Not a big deal, but I don't mind factory methods following private constructors.
And then you have non-factory public static methods which feels odd to be before constructors and member fields (again we are talking about java specifically). Anyway, up to you.

P.s. again this is old but still feels relevant: http://www.oracle.com/technetwork/java/codeconventions-141855.html

I'd rather keep it where it is so it's not buried.

kkonstantine · 2017-09-20T11:59:24Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequency.java

+     * @param centerValue the value identifying the {@link Frequencies} bucket to be reported
+     */
+    public Frequency(MetricName name, double centerValue) {
+        super();


class doesn't extend another class.

ack. Copied from Percentile, but removed now.

kkonstantine · 2017-09-20T11:59:43Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequency.java

+
+    /**
+     * Create an instance with the given name and center point value.
+     * @param name the name of the frequency metric; may not be null


missing blank line

kkonstantine · 2017-09-20T12:00:07Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequency.java

+        this.centerValue = centerValue;
+    }
+
+    public MetricName name() {


missing javadoc to public method

kkonstantine · 2017-09-20T12:00:14Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequency.java

+        return this.name;
+    }
+
+    public double centerValue() {


missing javadoc to public method

kkonstantine · 2017-09-20T12:55:18Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+        return count / (double) totalCount;
+    }
+
+    public double totalCount(MetricConfig config, long now) {


both arguments are unused

kkonstantine · 2017-09-20T12:56:59Z

clients/src/test/java/org/apache/kafka/common/metrics/stats/HistogramTest.java

+        assertEquals("Check boundary of bucket 3", 3, scheme.toBin(3.9999));
+        assertEquals("Check boundary of bucket 4", 4, scheme.toBin(4.0000));
+        assertEquals("Check boundary of bucket 4", 4, scheme.toBin(4.9999));
+        assertEquals("Check boundary of bucket 4", 4, scheme.toBin(5.000));


why precision is reduced in those two cases?

kkonstantine · 2017-09-20T13:14:41Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/StateTracker.java

+ * Utility class that tracks the current state and the duration of time spent in each state.
+ */
+public class StateTracker {
+    private volatile State currentState;


A couple of questions before I dive deeper into this if I have to.

These are a lot of volatile variables. Are we confident that the overall access pattern will be lighter than using a mutex (or combination of 1-2 volatiles with a mutex)?
Are the operations on the shared state composable? (see comment below). Are they somehow monotonically additive? (I wouldn't think so by glancing over durationRatio below)

You raise a great point, and the code you reviewed definitely is not valid. I've completely redesigned the whole StateTracker class to be properly and efficiently threadsafe, and it should address your concerns.

kkonstantine · 2017-09-20T13:15:29Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/StateTracker.java

+                durationDesired += destroyedTotalTimeMs;
+                break;
+        }
+        long total = durationCurrent + unassignedTotalTimeMs + runningTotalTimeMs + pausedTotalTimeMs +


Either this is not atomic or I'm missing something.

kkonstantine · 2017-09-20T13:18:13Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+            }
+        }
+
+        void recordCommit(long duration, boolean success, Throwable error) {


error is unused

I anticipate adding more metrics in the future that may want to know the error (e.g., last error reported by the task). I'd rather leave this as is.

rhauch · 2017-09-20T22:30:02Z

Rebased on the latest #3864.

Expanded upon the unit tests, and corrected several issues found during the expanded testing of these classes that don’t appear to have been used yet.

…asks

rhauch

@kkonstantine ok, #3864 has been merged and this PR is now rebased on trunk, so this is ready for a "real" review.

A number of changes were moved from this PR into that PR, resulting in kind of a mess with the previous commits. So, I squashed several of them and now have just 2 commits: one to fix the Percentiles and Histogram classes and tests (which were unused up till now), and the second larger one is to add tasks metrics common to both sources and sinks. Below are a number of line comments with commentary about the changes.
Thanks for reviewing!

rhauch · 2017-09-22T22:57:11Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

+        if (quantile > 1.00d)
+            return Float.POSITIVE_INFINITY;
+        if (quantile < 0.00d)
+            return Float.NEGATIVE_INFINITY;


The Histogram and Percentiles classes were not yet used in the codebase, but I want to use both in the Connect metrics. Neither was really working properly, so I expanded the incomplete HistorgramTest and discovered it was not behaving correctly nor consistently with how it was being used in Percentiles.

I agree. They're both pretty useful for metrics. Glad you discovered what was off with these constructs. And thanks for the explanation, now the changes in the formulas make more sense.

rhauch · 2017-09-22T22:59:26Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

@@ -92,29 +96,29 @@ public ConstantBinScheme(int bins, double min, double max) {
            this.min = min;
            this.max = max;
            this.bins = bins;
-            this.bucketWidth = (max - min) / (bins - 2);
+            this.bucketWidth = (max - min) / bins;


The LinearBinScheme and ConstantBinScheme were not consistent with each other or with how Histogram uses them. And, the logic simply didn't work, as the previous code did not even produce the correct number of bins.

rhauch · 2017-09-22T23:03:45Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

@@ -149,10 +156,8 @@ public int toBin(double x) {
            } else if (x > this.max) {
                return this.bins - 1;
            } else {
-                double scaled = x / this.scale;
-                return (int) (-0.5 + Math.sqrt(2.0 * scaled + 0.25));
+                return (int) (-0.5 + 0.5 * Math.sqrt(1.0 + 8.0 * x / this.scale));


Not really sure where the previous equation came from, and couldn't find any similar algorithm in papers or code. The new equation is simply a simplification of the quadratic formula that solves the second-order equation on line 149.

Since you mentioned, is there a single doc you used as a guide for Histogram's impl? (A wikipedia article maybe?) If there's something concrete, maybe would be useful to add it to the javadoc, or an inline non-javadoc comment for this class.

I'm not sure what served as the original inspiration, but the algorithm is really just returning bins whose widths are the sum of all prior bin widths. IOW, the equation on line 149 in fromBin method is really just n (n+1) / 2, aka Gauss' Summation Trick, scaled such that the final bin has a range that ends with max.

rhauch · 2017-09-22T23:04:59Z

clients/src/test/java/org/apache/kafka/common/metrics/stats/HistogramTest.java

    }

    @Test
    public void testConstantBinScheme() {
        ConstantBinScheme scheme = new ConstantBinScheme(5, -5, 5);
        assertEquals("A value below the lower bound should map to the first bin", 0, scheme.toBin(-5.01));
        assertEquals("A value above the upper bound should map to the last bin", 4, scheme.toBin(5.01));
-        assertEquals("Check boundary of bucket 1", 1, scheme.toBin(-5));
-        assertEquals("Check boundary of bucket 4", 4, scheme.toBin(5));
-        assertEquals("Check boundary of bucket 3", 3, scheme.toBin(4.9999));


Bin numbers are 0-based, so this was wrong to start with.

rhauch · 2017-09-22T23:09:41Z

clients/src/test/java/org/apache/kafka/common/metrics/stats/HistogramTest.java

+        assertEquals(-1.0, scheme.fromBin(2), 0.001d);
+        assertEquals(1.0, scheme.fromBin(3), 0.001d);
+        assertEquals(3.0, scheme.fromBin(4), 0.001d);
+        checkBinningConsistency(scheme);


Of course, it is possible to simplify these tests to use a loop, passing the output of fromBin() into toBin() and just checking they are consistent. That is what checkBinningConsistency does, and it like the name suggests it only checks that the two methods are consistent, not that the bins are computed correctly. IMO it is far clearer and more complete to use literal values and to check the range of the bins. I am intentionally using literals with different precision.

Makes sense.

rhauch · 2017-09-22T23:11:07Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/StateTracker.java

+ */
+public class StateTracker {
+
+    private final AtomicReference<StateChange> lastState = new AtomicReference<>(new StateChange());


@kkonstantine, on a previous review you caught how this class was not threadsafe. This new version is threadsafe and only requires on synchronized method only when the state (infrequently) changes.

rhauch · 2017-09-22T23:14:00Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/StateTracker.java

+     */
+    public synchronized void changeState(State newState, long now) {
+        // JDK8: remove synchronization by using lastState.getAndUpdate(oldState->oldState.newState(newState, now));
+        lastState.set(lastState.get().newState(newState, now));


When we switch to Java 8 as the minimum compilation level, we can/should remove the synchronization and replace this method to use the atomic AtomicReference.getAndUpdate(UnaryOperator<T> updateFunction) method.

Sure, that's cool.

If you want immediately what JDK8 will bring with getAndUpdate, you may write this as:

StateChange current; do { current = lastState.get(); } while (!lastState.compareAndSet(current, current.newState(newState, now)));

but a one-liner with synchronized here is fine too.

rhauch · 2017-09-22T23:15:40Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+     */
+    protected void recordCommitFailure(long duration, Throwable error) {
+        taskMetricsGroup.recordCommit(duration, false, error);
+    }


I actually had a single recordCommit(long duration, boolean success, Throwable error) method instead of the above 2 methods, but these two methods are IMO more readable where they're used and are therefore preferable.

rhauch · 2017-09-22T23:18:19Z

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerSinkTaskTest.java

        workerTask = PowerMock.createPartialMock(
                WorkerSinkTask.class, new String[]{"createConsumer"},
-                taskId, sinkTask, statusListener, initialState, workerConfig, keyConverter, valueConverter, transformationChain, pluginLoader, time);
+                taskId, sinkTask, statusListener, initialState, workerConfig, metrics, keyConverter, valueConverter, transformationChain, pluginLoader, time);
+    }


Unfortunately it was necessary to not create multiple WorkerTask objects with the same name, since that results in extra metrics that pose problems in the test with the mocks and expectations. That's an unrealistic situation, so this change, while unappealing, is the easiest way to prevent that.

kkonstantine

Thanks @rhauch!

Looks good to me now. Left a few comments as replies, but I don't see anything additional that needs to be addressed here.

LGTM!

kkonstantine · 2017-09-25T10:42:53Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

+        if (quantile > 1.00d)
+            return Float.POSITIVE_INFINITY;
+        if (quantile < 0.00d)
+            return Float.NEGATIVE_INFINITY;


I agree. They're both pretty useful for metrics. Glad you discovered what was off with these constructs. And thanks for the explanation, now the changes in the formulas make more sense.

kkonstantine · 2017-09-25T10:46:02Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

@@ -149,10 +156,8 @@ public int toBin(double x) {
            } else if (x > this.max) {
                return this.bins - 1;
            } else {
-                double scaled = x / this.scale;
-                return (int) (-0.5 + Math.sqrt(2.0 * scaled + 0.25));
+                return (int) (-0.5 + 0.5 * Math.sqrt(1.0 + 8.0 * x / this.scale));


Since you mentioned, is there a single doc you used as a guide for Histogram's impl? (A wikipedia article maybe?) If there's something concrete, maybe would be useful to add it to the javadoc, or an inline non-javadoc comment for this class.

kkonstantine · 2017-09-25T10:46:51Z

clients/src/test/java/org/apache/kafka/common/metrics/stats/FrequenciesTest.java

+        MetricName name3 = name("3");
+        MetricName name4 = name("4");
+        Frequencies frequencies = new Frequencies(4, 1.0, 4.0,
+                                                         new Frequency(name1, 1.0),


nit: Is alignment correct here?

Nope. One sec.

kkonstantine · 2017-09-25T10:48:03Z

clients/src/test/java/org/apache/kafka/common/metrics/stats/HistogramTest.java

+        assertEquals(-1.0, scheme.fromBin(2), 0.001d);
+        assertEquals(1.0, scheme.fromBin(3), 0.001d);
+        assertEquals(3.0, scheme.fromBin(4), 0.001d);
+        checkBinningConsistency(scheme);


Makes sense.

kkonstantine · 2017-09-25T11:12:02Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/StateTracker.java

+     */
+    public synchronized void changeState(State newState, long now) {
+        // JDK8: remove synchronization by using lastState.getAndUpdate(oldState->oldState.newState(newState, now));
+        lastState.set(lastState.get().newState(newState, now));


Sure, that's cool.

If you want immediately what JDK8 will bring with getAndUpdate, you may write this as:

StateChange current; do { current = lastState.get(); } while (!lastState.compareAndSet(current, current.newState(newState, now)));

but a one-liner with synchronized here is fine too.

rhauch · 2017-09-25T18:12:12Z

@kkonstantine, I just added some JavaDoc to the Histogram class and fixed the indentation you mentioned in the FrequenciesTest class.

@ewencp, I'll ping you for this since you're the committer that did the review on #3864.

kkonstantine

LGTM

ewencp · 2017-09-25T20:38:42Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+ * object is a {@link CompoundStat}, and so it can be {@link org.apache.kafka.common.metrics.Sensor#add(CompoundStat)
+ * added directly to a Sensor} so the metrics are created automatically.
+ */
+public class Frequencies extends SampledStat implements CompoundStat {


This is new because the existing -ratio metrics are Rates and have actual time units/measurements that are actual times rather than just samples?

Yes, they're durations over total time. This class captures a normal histogram of values, but unlike Percentiles does not aggregate the counts below the requested bin.

ewencp · 2017-09-25T20:45:29Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+     *                                  {@link Frequency#centerValue() center value} within the specified range
+     */
+    public Frequencies(int buckets, double min, double max, Frequency... frequencies) {
+        super(0.0);


Why is this the right initial value? What if 0.0 isn't even in the range specified by min/max?

The base class uses it to create a new Sample, and this class doesn't use that initial value in the samples. So, that value is unused. I can add a comment.

ewencp · 2017-09-25T20:52:16Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+            throw new IllegalArgumentException("Must be at least 2 buckets");
+        }
+        if (buckets < frequencies.length) {
+            throw new IllegalArgumentException("More frequencies than buckets");


Isn't the thing that matters whether there is more than one frequency per bucket?

It's important that all supplied Frequency objects have a bin. I don't think having more than one Frequency per bin is horrible; yes, it's a coding error, so I can add a check.

ewencp · 2017-09-25T20:52:55Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Frequencies.java

+            }
+        }
+        double halfBucketWidth = (max - min) / (buckets - 1) / 2.0;
+        this.binScheme = new ConstantBinScheme(buckets, min - halfBucketWidth, max + halfBucketWidth);


This actually got quite complex for just counting the frequency of 2 fixed values. Is all of this really needed? Are we going to use the more complicated functionality in a later patch?

I do plan on using it for more complex cases as well. And, I started writing something that applied to boolean samples, and it wasn't that much more efficient but would have resulted in quite a bit of duplicate code. Plus, SampledStat already comes with a fair amount of baggage (e.g., the record method takes only double values, Sample has several values, samples are List<Sample>, etc.), so using it with boolean wouldn't be simpler. Given all of this, I thought using this for boolean values was an acceptable compromise.

ewencp · 2017-09-25T21:23:35Z

clients/src/main/java/org/apache/kafka/common/metrics/stats/Histogram.java

                return bins - 1;
            else
-                return (int) ((x - min) / bucketWidth) + 1;
+                return (int) ((x - min) / bucketWidth);


are there ever any issues with floating point precision here and the bucketWidth? Should this clamp to 0 to bins -1 just to be safe?

Probably wouldn't hurt. I'll add that.

ewencp · 2017-09-25T22:07:23Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+        private final Sensor commitAttempts;
+
+        public TaskMetricsGroup(ConnectorTaskId id, ConnectMetrics connectMetrics, TaskStatus.Listener statusListener) {
+            delegateListener = statusListener;


if we keep ending up with this pattern, it might be clearer to create a Listener implementation that delegates to a list of listeners instead of chaining them manually this way

Yeah, but for just 2 listeners I didn't think it was worth it (yet).

ewencp · 2017-09-25T22:08:02Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+            addTaskStateMetric(State.DESTROYED, "status-destroyed",
+                    "Signals whether the connector task is in the destroyed state.");
+
+            addRatioMetric(State.RUNNING, "running-ratio",


this state is missing from the KIP, it should be added

Ah, good catch. Fixed.

ewencp · 2017-09-25T22:08:10Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+                    "Signals whether the connector task is in the destroyed state.");
+
+            addRatioMetric(State.RUNNING, "running-ratio",
+                    "The fraction of time this task has spent in the paused state.");


paused -> running

ewencp · 2017-09-25T22:14:39Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+                    "The average time in milliseconds taken by this task to commit offsets"),
+                    new Avg());
+
+//            int buckets = 100;


is this all commented out intentionally?

We will want them at some point, but I wasn't sure about the correctness of Percentiles and these are not high priority metrics. Turns out I think at least some of the problem was in Histogram, which I've since fixed. This is in a commit now, so if you prefer I can remove them.

we just generally try to avoid the mess if the code isn't ready to be committed yet. it can always be held onto in a branch, wip pr, etc

ewencp · 2017-09-25T22:20:50Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+
+        @Override
+        public void onShutdown(ConnectorTaskId id) {
+            taskStateTimer.changeState(State.UNASSIGNED, time.milliseconds());


I don't see sensors being removed, but for tasks we'd definitely want to remove them when the task gets reassigned to another worker. I haven't thought it through, but this might be the right place to be doing that rather than marking it unassigned.

Relatedly, I think there might be some sort of checks in unit tests in maybe the producer or consumer that validate metrics are unregistered, might be able to use a similar approach here.

Great point about removing. I completely missed that, so I'll address that now.

… tasks are stopped/removed The ability to easily remove the metrics required a few more changes to the `ConnectMetrics` class, including the addition of methods to add `Sensor` objects.

The sensor names used within a group must be unique, so now the group creates sensors using a prefix that includes the group’s name and tags, which must be unique to the group.

rhauch · 2017-09-26T20:30:22Z

@ewencp, I added two more commit. The first addresses all of the concerns you mentioned above. Unfortunately, removing the sensors was not trivial, so it required a few more methods on the MetricGroup to hold on to the sensor names so they can be removed. All metrics that are part of the group are all automatically removed.

The second commit ensures that MetricGroup are unique and keyed by the group name and the tags. This is essential if each task is to have its own metric groups.

ewencp

@rhauch Getting close, one real issue w/ ordering of tags but otherwise just a few minor cleanups.

ewencp · 2017-09-26T23:04:43Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectMetrics.java

+            assert groupName != null;
+            assert tags != null;
+            this.groupName = groupName;
+            this.tags = Collections.unmodifiableMap(new HashMap<>(tags));


I think this should ensure it preserves the order of the tags.

ewencp · 2017-09-26T23:08:03Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectMetrics.java

            if (previous != null) group = previous;
        }
        return group;
    }

+    protected MetricGroupId groupId(String groupName, boolean includeWorkerId, String... tagKeyValues) {
+        checkNameIsValid(groupName);
+        Map<String, String> tags = tags(includeWorkerId ? workerId : null, tagKeyValues);


We missed it in the first round probably because those metrics only had 1 tag, but the tags method should use something like a LinkedHashMap. Without this, the tags can get jumbled. I noticed this because jconsole's hierarchy was listing the task ID above the connector name.

ewencp · 2017-09-26T23:13:13Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSourceTask.java

@@ -124,6 +125,10 @@ protected void close() {
    }

    @Override
+    protected void releaseResources() {


Can we just drop this since neither implementation contains any code?

Actually, this will be used in the future PRs to close the subclass-specific metrics. I debated about doing this inside close, but the problem is that we'd miss any failures that happen when the tasks are being closed.

ewencp · 2017-09-26T23:15:24Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

+                    "The average time in milliseconds taken by this task to commit offsets"),
+                    new Avg());
+
+//            int buckets = 100;


we just generally try to avoid the mess if the code isn't ready to be committed yet. it can always be held onto in a branch, wip pr, etc

ewencp · 2017-09-26T23:20:57Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectMetrics.java

+         *
+         * @param sensors The sensors to be removed
+         */
+        public synchronized void removeSensors(Sensor... sensors) {


This isn't currently used. Are we expecting to have a use case for it or should we just remove it? Generally you'd only remove the entire group together and only when it is being destroyed, right?

It is not being used. I can remove it.

…other cleanup

rhauch · 2017-09-27T01:53:32Z

@ewencp another commit to address your comments/suggestions.

ewencp

LGTM

Added Connect metrics specific to source tasks, and builds upon #3864 and #3911 that have already been merged into `trunk`. Author: Randall Hauch <rhauch@gmail.com> Reviewers: tedyu <yuzhihong@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #3959 from rhauch/kafka-5901

Added Connect metrics specific to source tasks, and builds upon #3864 and #3911 that have already been merged into `trunk`, and #3959 that has yet to be merged. I'll rebase this PR when the latter is merged. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #3975 from rhauch/kafka-5902

rhauch changed the title ~~Kafka 5900~~ KAFKA-5900 Add task metrics common to both sink and source tasks Sep 19, 2017

rhauch changed the title ~~KAFKA-5900 Add task metrics common to both sink and source tasks~~ [WIP] KAFKA-5900: Add task metrics common to both sink and source tasks Sep 19, 2017

kkonstantine reviewed Sep 20, 2017

View reviewed changes

rhauch force-pushed the kafka-5900 branch from d1b7967 to e22be05 Compare September 20, 2017 18:13

rhauch mentioned this pull request Sep 20, 2017

KAFKA-5899: Added Connect metrics for connectors #3864

Closed

rhauch force-pushed the kafka-5900 branch 2 times, most recently from 476b89c to 2c7d75a Compare September 20, 2017 22:30

rhauch force-pushed the kafka-5900 branch from 2c7d75a to 9760d7c Compare September 22, 2017 20:53

rhauch added 2 commits September 22, 2017 17:15

KAFKA-5900 Corrected Percentiles and Histogram metric components

2b96e8b

Expanded upon the unit tests, and corrected several issues found during the expanded testing of these classes that don’t appear to have been used yet.

KAFKA-5900 Added worker task metrics common to both sink and source t…

7563c14

…asks

rhauch force-pushed the kafka-5900 branch from 9760d7c to 7563c14 Compare September 22, 2017 22:47

rhauch commented Sep 22, 2017

View reviewed changes

kkonstantine approved these changes Sep 25, 2017

View reviewed changes

KAFKA-5900 Added JavaDoc and improved indentation

418ec59

rhauch changed the title ~~[WIP] KAFKA-5900: Add task metrics common to both sink and source tasks~~ KAFKA-5900: Add task metrics common to both sink and source tasks Sep 25, 2017

kkonstantine approved these changes Sep 25, 2017

View reviewed changes

rhauch mentioned this pull request Sep 25, 2017

KAFKA-5901: Added Connect metrics specific to source tasks #3959

Closed

ewencp reviewed Sep 25, 2017

View reviewed changes

rhauch added 2 commits September 26, 2017 12:22

KAFKA-5900 Added support for removing the metrics when connectors and…

e2961e4

… tasks are stopped/removed The ability to easily remove the metrics required a few more changes to the `ConnectMetrics` class, including the addition of methods to add `Sensor` objects.

KAFKA-5900 Changed metric groups to use unique sensor names

9ef7ea7

The sensor names used within a group must be unique, so now the group creates sensors using a prefix that includes the group’s name and tags, which must be unique to the group.

ewencp reviewed Sep 26, 2017

View reviewed changes

KAFKA-5900 Maintained order of tags in metric group identifiers, and …

0b8d8cb

…other cleanup

ewencp approved these changes Sep 27, 2017

View reviewed changes

asfgit closed this in 73cc416 Sep 27, 2017

rhauch mentioned this pull request Sep 27, 2017

KAFKA-5902: Added sink task metrics #3975

Closed

kkonstantine added the connect label Oct 16, 2020

yashmayya mentioned this pull request Jun 29, 2023

MINOR: Replace synchronization with atomic update in Connect's StateTracker::changeState method #13934

Merged

3 tasks

KAFKA-5900: Add task metrics common to both sink and source tasks #3911

KAFKA-5900: Add task metrics common to both sink and source tasks #3911

Conversation

rhauch commented Sep 19, 2017 • edited Loading

rhauch commented Sep 19, 2017

kkonstantine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhauch Sep 20, 2017 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhauch commented Sep 20, 2017

rhauch left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkonstantine left a 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

rhauch Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhauch commented Sep 25, 2017

kkonstantine left a 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

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

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

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

rhauch commented Sep 19, 2017 •

edited

Loading

rhauch Sep 20, 2017 •

edited

Loading

rhauch left a comment •

edited

Loading

rhauch Sep 25, 2017 •

edited

Loading