-
Notifications
You must be signed in to change notification settings - Fork 331
Improvements to MetricsSnapshotReporter #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /* | ||
| * 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.samza.metrics; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.LinkedList; | ||
|
|
||
|
|
||
| /** | ||
| * A ListGauge is a collection of {@link org.apache.samza.metrics.Gauge}. ListGauges are useful for maintaining, recording, | ||
| * or collecting specific Gauge values over time. It is implemented as a {@link org.apache.samza.metrics.Metric}. | ||
| * For example, the set of recent errors that have occurred. | ||
| * | ||
| * This current implementation uses a size-bound-policy and holds the N most-recent Gauge objects added to the list. | ||
| * This bound N is configurable at instantiation time. | ||
| * TODO: Support a time-based and size-and-time-based hybrid policy. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this PR is only adding gauges, maybe move the TODOs to another JIRA and assign them to yourself. You can remove these from code.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| * TODO: Add a derived class to do compaction for errors using hash-based errorIDs and adding timestamp for errors to dedup | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should be a new metric type then? would be good to think what the end-user API is so that we build the right thing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, agree |
||
| * on the read path. | ||
| * | ||
| * All public methods are thread-safe. | ||
| * | ||
| */ | ||
| public class ListGauge implements Metric { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about this as a list of values? You don't have to expose the fact that it uses a Gauge underneath. ListGauge merely takes in an add(T val)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to parameterize this class?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| private final String name; | ||
| private final Collection<Gauge> metricList; | ||
| private int nItems; | ||
|
|
||
| /** | ||
| * Create a new ListGauge. | ||
| * @param name Name to be assigned | ||
| * @param nItems The number of items to hold in the list | ||
| */ | ||
| public ListGauge(String name, int nItems) { | ||
| this.name = name; | ||
| this.metricList = new LinkedList<>(); | ||
| this.nItems = nItems; | ||
| } | ||
|
|
||
| /** | ||
| * Get the name assigned to the ListGauge | ||
| * @return the assigned name | ||
| */ | ||
| public String getName() { | ||
| return this.name; | ||
| } | ||
|
|
||
| /** | ||
| * Add a gauge to the list | ||
| * @param value The Gauge value to be added | ||
| */ | ||
| public synchronized void add(Gauge value) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can just take in a T value
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| if (this.metricList.size() == nItems) { | ||
| ((LinkedList<Gauge>) this.metricList).removeFirst(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably don't need this cast here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } | ||
|
|
||
| this.metricList.add(value); | ||
| } | ||
|
|
||
| /** | ||
| * Get the Collection of Gauge values currently in the list | ||
| * @return the collection of gauge values | ||
| */ | ||
| public synchronized Collection<Gauge> getValue() { | ||
| return this.metricList; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we return a copy or wrap it in an immutable collection so that modifications don't alter the original?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public synchronized void visit(MetricsVisitor visitor) { | ||
| visitor.listGauge(this); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /* | ||
| * 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.samza.metrics; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.Collection; | ||
| import java.util.Iterator; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
|
|
||
| /** | ||
| * Class to encapsulate test-cases for {@link org.apache.samza.metrics.ListGauge} | ||
| */ | ||
| public class TestListGauge { | ||
|
|
||
| private final static Duration THREAD_TEST_TIMEOUT = Duration.ofSeconds(10); | ||
|
|
||
| @Test | ||
| public void basicTest() { | ||
| ListGauge listGauge = new ListGauge("listGauge", 10); | ||
| Gauge<String> sampleGauge = new Gauge<String>("key", "value"); | ||
| listGauge.add(sampleGauge); | ||
| Assert.assertEquals("Names should be the same", listGauge.getName(), "listGauge"); | ||
| Assert.assertEquals("List sizes should match", listGauge.getValue().size(), 1); | ||
| Assert.assertEquals("ListGauge should contain sampleGauge", listGauge.getValue().contains(sampleGauge), true); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSizeEnforcement() { | ||
| ListGauge listGauge = new ListGauge("listGauge", 10); | ||
| for (int i = 15; i > 0; i--) { | ||
| Gauge<String> sampleGauge = new Gauge<String>("key", "v" + i); | ||
| listGauge.add(sampleGauge); | ||
| } | ||
| Assert.assertEquals("List sizes should be as configured at creation time", listGauge.getValue().size(), 10); | ||
|
|
||
| int valueIndex = 10; | ||
| Collection<Gauge> currentList = listGauge.getValue(); | ||
| Iterator iterator = currentList.iterator(); | ||
| while (iterator.hasNext()) { | ||
| Gauge<String> gauge = (Gauge<String>) iterator.next(); | ||
| Assert.assertTrue(gauge.getName().equals("key")); | ||
| Assert.assertTrue(gauge.getValue().equals("v" + valueIndex)); | ||
| valueIndex--; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testThreadSafety() throws InterruptedException { | ||
| ListGauge listGauge = new ListGauge("listGauge", 20); | ||
|
|
||
| Thread thread1 = new Thread(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| for (int i = 1; i <= 100; i++) { | ||
| listGauge.add(new Gauge<Integer>("thread1", i)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Thread thread2 = new Thread(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| for (int i = 1; i <= 100; i++) { | ||
| listGauge.add(new Gauge<Integer>("key", i)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| thread1.start(); | ||
| thread2.start(); | ||
|
|
||
| thread1.join(THREAD_TEST_TIMEOUT.toMillis()); | ||
| thread2.join(THREAD_TEST_TIMEOUT.toMillis()); | ||
|
|
||
| Assert.assertTrue("ListGauge should have the last 20 values", listGauge.getValue().size() == 20); | ||
| for (Gauge gauge : listGauge.getValue()) { | ||
| Assert.assertTrue("Values should have the last 20 range", | ||
| ((Integer) gauge.getValue()) <= 100 && ((Integer) gauge.getValue()) > 80); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-wrote this slightly:
"A {@link Metric} that stores a collection of values. By default, a {@link ListGauge} retains the {@code size} most recent values"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed these comments in new, separate PR: #541