SAMZA-1733 New metric type Listgauge #541

rayman7718 · 2018-05-31T22:52:36Z

This PR introduces a ListGauge type,
A subsequent PR: #543 shows how this issued in conjunction with a diagnostics appender for error-tracking and dumpting to kafka via SnapshotReporter.

…pshot reporter. Switched producer-shutdown logic to ensure metric-flush at shutdown-time

…lt policy to retain last N

Added exception metric (as string-guage), to be emitted using the snapshot reporter. Switched producer-shutdown logic to ensure metric-flush at shutdown-time Adding ListGauge and integration with SamzaContainerMetric Typifying ListGauge to ListGauge<T>, adding an eviction policy, default policy to retain last N

…listgauge

cameronlee314

Could you please add some context to the description about how you plan to use this? One of the javadocs gives an example about errors, but are there other use cases and how do you plan to expose this data?

cameronlee314 · 2018-06-01T20:52:50Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+    }
+  }
+
+  public synchronized boolean remove(T value) {


Do you have a use case for being able to remove items from the middle of the list, or other arbitrary eviction? Could this maybe go a simpler route and have a bounded queue with a max size instead of a list with an arbitrary removal/eviction policy?

There is a usecase for time-based and size-based eviction, thats why we had to separate out the eviction policy logic, it could be all baked into this class, in which remove(T val) can be removed.

For both time and size-based eviction, it seems like you wouldn't need arbitrary removal. Wouldn't you just need a removeHead()?

Would prefer wrapping the eviction logic in custom-evictors and getting rid off the remove method

cameronlee314 · 2018-06-01T20:55:20Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * Package-private method to change the eviction policy
+   * @param listGaugeEvictionPolicy
+   */
+  synchronized void setEvictionPolicy(ListGaugeEvictionPolicy<T> listGaugeEvictionPolicy) {


Would it be possible to set this using a constructor instead of a setter? Then you don't need to have a synchronized method for setting it.

The problem was that the policy needed a handle on the listgauge, so it couldnt be created and set during the listgauge instantiation.

Maybe can the callback instead accept the ListGauge? It would be nice to avoid this mutability and a circular dependency.

Simplified the impl by removing configurability since there is no use case for it yet.

cameronlee314 · 2018-06-01T21:03:30Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * {@inheritDoc}
+   */
+  @Override
+  public synchronized void visit(MetricsVisitor visitor) {


Do all these instance methods need to be synchronized? Could you use some "concurrent" data structure instead? The main thing I am worried about is this visit method which can do arbitrary logic and possibly hold on to the lock for an uncontrolled period of time.

yup, looking into using concurrentlinkedqueue/deque, but the
current concurrency and periodicity scale for this are very low, for example, visit gets invoked once a minute

Every time that minute comes around though, all other methods on the ListGauge have to wait for visit to complete. For example, if visit makes a remote call somewhere, then all add methods have to wait for that remote call to finish.

Simplified by using a concurrent queue.

cameronlee314 · 2018-06-01T21:48:23Z

samza-api/src/main/java/org/apache/samza/metrics/RetainLastNPolicy.java

+    Iterator<T> iterator = listGaugeCollection.iterator();
+    while (numToEvict > 0 && iterator.hasNext()) {
+      // Remove in FIFO order to retain the last nItems
+      listGauge.remove(iterator.next());


It looks like this is an n^2 removal, since you are always removing from the beginning of the list. Is it possible to optimize it?

Can there be problems with iterating over the listGauge values and removing from the listGauge at the same time? It looks like Collections.unmodifiableList doesn't make a copy of the list.

yes, an easy way is to bake in the eviction logic and expose only control-knows #items or durationThreshold, and remove policy as configurable, and get rid of remove altogether.

unmodifiableList doesnt give a copy of the elements but it does give a copy of the references to the elements, and listGauge.remove is sync.
so for most immutable elements (for which this was intended), this could work

Above you mentioned that you would like to keep remove, but here it sounds like you are suggesting to get rid of remove. Could you please clarify?

I wasn't referring to multithreaded concurrency in this case. My concern was that the iterator you are iterating over in the while loop and the listGauge that you are calling remove on seem to be both backed by the same underlying list object. I was wondering if the removal can cause some inconsistency in the iterator. It could be helpful to have a removeHead() here too so you don't need to actually iterate through the elements.

cameronlee314 · 2018-06-01T22:00:23Z

samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala

        registry.getGroup(group).asScala.foreach {
          case (name, metric) =>
            metric.visit(new MetricsVisitor {
+              // for listGauge the value is returned as a list, which gets serialized


I think you might need to update AmfReporter.java too.

Sure, will do.
The plan was also to use AmfReporter's filter to filter out the "exception" metric, because amf cant handle exceptions/string types

vjagadish1989 · 2018-06-02T00:06:41Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+ */
+public class ListGauge<T> implements Metric {
+  private final String name;
+  private final List<T> metricList;


s/metricList/elements

vjagadish1989 · 2018-06-02T00:11:00Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+    }
+  }
+
+  public synchronized boolean remove(T value) {


Would prefer wrapping the eviction logic in custom-evictors and getting rid off the remove method

vjagadish1989 · 2018-06-02T00:11:48Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * Get the Collection of Gauge values currently in the list, used when serializing this Gauge.
+   * @return the collection of gauge values
+   */
+  public synchronized Collection<T> getValue() {


s/getValue/getValues

vjagadish1989 · 2018-06-02T00:13:19Z

samza-api/src/main/java/org/apache/samza/metrics/ListGaugeEvictionPolicy.java

+ */
+package org.apache.samza.metrics;
+
+public interface ListGaugeEvictionPolicy<T> {


Prefer to rethink this interface as follows:

public class Evictor { public void evict(List<T> elements); }

Evictors can mutate the list they are passed. In addition to getting rid off the setter, you should be able to support arbitrary eviction policies

talked offline

rayman7718 · 2018-06-02T01:05:21Z

I have a separate PR that shows how this abstraction is used for error-tracking.
#543

… only top-level params

vjagadish1989 · 2018-06-04T22:44:43Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+    this.elements.add(new ValueInfo<T>(Instant.now(), value));
+
+    // notify the policy object for performing any eviction that may be needed.
+    this.listGaugeEvictionPolicy.elementAddedCallback();


Now that there is a need for only 2 types of evictions (size, time), I wonder if we could just do evictions
inline on add? We wouldn't need a separate interface then. This would be the preferable approach.

If you still think we need an interface for a pluggable eviction policy, it could be something on the lines of:

public class EvictionPolicy<T> { void evict(Iterable<TimestampedValue<T>> currentElements); }

instead of

public class ListGaugeEvictionPolicy<T> { public void elementAddedCallback(); }```

done, purged the interface and just used the default class impl

vjagadish1989 · 2018-06-04T22:48:22Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * This class is used for bookkeeping of values added to the ListGauge.
+   * @param <T>
+   */
+  public static class ValueInfo<T> {


You wouldn't need this class and can use TimestampedValue instead

rayman7718

Purged the interfaces to simplify current impl,
removed ValueInfo, and moved TimestampedInfo to samza-api

Removing ValueInfo class and moving TimestampedValue to samza-api util Adding TimestampedValue.java in core

…listgauge

vjagadish1989 · 2018-06-05T16:48:09Z

@rayman7718 : I meant that you don't need the interface and the corresponding implementations. I was proposing to inline the logic inside the ListGauge itself. We don't need the ability to schedule evictions asynchronously - we can instead trigger them when a new element is added.

…istGauge

vjagadish1989 · 2018-06-06T03:36:32Z

samza-api/src/main/java/org/apache/samza/metrics/DefaultListGaugeEvictionPolicy.java

+   * Evicts entries from the elements list, based on the given item-size and durationThreshold.
+   * Callers are responsible for thread-safety.
+   */
+  public void evict(Queue<TimestampedValue<T>> elements, int maxNumberOfItems, Duration maxStaleness) {


wondering if we could get rid off the eviction policy class as well and move this inside the ListGauge itself.

…ic into listgauge

rayman7718 · 2018-06-06T21:40:10Z

Completely removed evictionpolicy separation, merged all eviction logic into listgauge

vjagadish1989

one more pass!

vjagadish1989 · 2018-06-07T04:09:45Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+ * {@link ListGauge}s are useful for maintaining, recording, or collecting values over time.
+ * For example, a set of specific logging-events (e.g., errors).
+ *
+ * Eviction from list is either done by consuming-code using the remove APIs or by specifying an eviction policy


nuke L:36 and update it so that it's inline with the current implementation

vjagadish1989 · 2018-06-07T04:12:52Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+  }
+
+  /**
+   * Get the Collection of Gauge values currently in the list, used when serializing this Gauge.


Returns an immutable copy of the values in this {@link ListGauge}

don't need the comment on "Also evicts values based on the configured maxItems and maxStaleness."

vjagadish1989 · 2018-06-07T04:13:59Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * @return the collection of gauge values
+   */
+  public Collection<T> getValues() {
+    // notify the policy object for performing any eviction that may be needed.


delete this comment since the code has changed?

vjagadish1989 · 2018-06-07T04:14:29Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * Evicts entries from the elements list, based on the given item-size and durationThreshold.
+   * Callers are responsible for thread-safety.
+   */
+  public void evict(Queue<TimestampedValue<T>> elements, int maxNumberOfItems, Duration maxStaleness) {


why is this a public method? can this be internal to the gauge?

vjagadish1989 · 2018-06-07T04:14:52Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+   * Callers are responsible for thread-safety.
+   */
+  public void evict(Queue<TimestampedValue<T>> elements, int maxNumberOfItems, Duration maxStaleness) {
+    this.evictBasedOnSize(elements, maxNumberOfItems);


don't ned to pass in elements, maxNum, staleness - should be simpler to instead use the data members of the class

vjagadish1989 · 2018-06-07T04:15:15Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+
+  /**
+   * Evicts entries from the elements list, based on the given item-size and durationThreshold.
+   * Callers are responsible for thread-safety.


contradicts the ListGauge docs a little bit since public methods are expected to be thread-safe?

vjagadish1989 · 2018-06-07T04:16:22Z

samza-api/src/test/java/org/apache/samza/metrics/TestTimer.java

    assertEquals(3, snapshot.getValues().size());

-    // The time is 500 for update(4L) because getSnapshot calls clock once + 3
+    // The time is 500 for update(4L) because getValues calls clock once + 3


revert this?

vjagadish1989 · 2018-06-07T04:16:33Z

samza-azure/src/test/java/org/apache/samza/system/eventhub/TestMetricsRegistry.java

+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *


Revert unnecessary changes here (and everywhere else)
nit 1: revert changes to license header formats
nit 2: revert superfluous new lines in other classes

vjagadish1989

approved! thank you Rayman for the contribution

vjagadish1989 · 2018-06-08T21:10:36Z

samza-azure/src/test/java/org/apache/samza/system/eventhub/TestMetricsRegistry.java

+  @Override
+  public ListGauge newListGauge(String group, ListGauge listGauge) {
+    listGauges.putIfAbsent(group, new ArrayList());
+    ListGauge value = new ListGauge(group);


s/value/listGauge

vjagadish1989 · 2018-06-08T21:12:22Z

samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java

  }

+  public <T> ListGauge newListGauge(String name) {
+    return registry.newListGauge(groupName, new ListGauge(name));


should you do a ListGauge<T> for explicit type inference here since the method is parameterized by T already?

vjagadish1989 · 2018-06-08T21:13:15Z

samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala


  val taskStoreRestorationMetrics: util.Map[TaskName, Gauge[Long]] = new util.HashMap[TaskName, Gauge[Long]]()

+  // A string-gauge metric to capture exceptions at this container


s/exception/exceptions

minor: can nuke comment since it's obvious

vjagadish1989 · 2018-06-08T21:17:01Z

samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala


-    producer.stop
+    // Scheduling an event with 0 delay to ensure flushing of metrics one last time before shutdown
+    executor.schedule(this,0, TimeUnit.SECONDS)


is this a "best-effort" flush? if not, we should probably schedule this from the current thread synchronously. I believe a best effort flush might be good enough for most cases since the timeout is 60s

vjagadish1989 · 2018-06-08T21:19:22Z

samza-api/src/main/java/org/apache/samza/metrics/ListGauge.java

+
+
+/**
+ * A {@link ListGauge} is a {@link org.apache.samza.metrics.Metric} that buffers multiple instances of a type T in a list.


minor: Do you need the fully qualified name "{@link org.apache.samza.metrics.Metric}" here? it seems like you should be able to use {@link Metric}

vjagadish1989 · 2018-06-13T22:48:39Z

@rayman7718 : can we change the title to be something on the lines of:
"SAMZA-1234: Add .."

vjagadish1989 · 2018-06-14T04:03:01Z

merged and submitted. Thanks @rayman7718!

This PR introduces a ListGauge type, A subsequent PR: apache#543 shows how this issued in conjunction with a diagnostics appender for error-tracking and dumpting to kafka via SnapshotReporter. Author: Ray Matharu <[email protected]> Reviewers: Jagadish <[email protected]>, Cameron Lee <[email protected]> Closes apache#541 from rayman7718/listgauge

rmatharu added 5 commits May 31, 2018 15:44

Added exception metric (as string-guage), to be emitted using the sna…

5ae1df9

…pshot reporter. Switched producer-shutdown logic to ensure metric-flush at shutdown-time

Adding ListGauge and integration with SamzaContainerMetric

5a3ef7b

Typifying ListGauge to ListGauge<T>, adding an eviction policy, defau…

75ef5d2

…lt policy to retain last N

Merge branch 'listgauge' of https://github.com/rayman7718/samza into …

d2922c5

…listgauge

rayman7718 mentioned this pull request May 31, 2018

Improvements to MetricsSnapshotReporter #518

Closed

cameronlee314 reviewed Jun 1, 2018

View reviewed changes

vjagadish1989 suggested changes Jun 2, 2018

View reviewed changes

rmatharu added 2 commits June 4, 2018 13:03

Removing remove() and policy-configurability from ListGauge, exposing…

1f08eae

… only top-level params

Renaming valueList to elements, and getValue to getValues

6f19ba0

vjagadish1989 suggested changes Jun 4, 2018

View reviewed changes

rmatharu added 2 commits June 4, 2018 17:28

Removing the interface for ListGaugeEvictionPolicy

4f08cd1

Removing ValueInfo class and moving TimestampedValue to samza-api util

bdc85d4

rayman7718 commented Jun 5, 2018

View reviewed changes

rmatharu added 2 commits June 4, 2018 18:33

Removing ValueInfo class and moving TimestampedValue to samza-api util

5c1ed0a

Removing ValueInfo class and moving TimestampedValue to samza-api util Adding TimestampedValue.java in core

Merge branch 'listgauge' of https://github.com/rayman7718/samza into …

c1884e4

…listgauge

rmatharu added 2 commits June 5, 2018 11:42

Removing periodic pruning from Listgauge, pruning on add and get in L…

c7821ba

…istGauge

Updating testListGauge for new listgauge

9ba8e5b

vjagadish1989 suggested changes Jun 6, 2018

View reviewed changes

Complete removing evictionpolicy separation, merging all eviction log…

fd77c53

…ic into listgauge

vjagadish1989 suggested changes Jun 7, 2018

View reviewed changes

Reverting superflous comment changes, indentation changes

cc4d881

vjagadish1989 approved these changes Jun 8, 2018

View reviewed changes

Minor changes

863d850

rayman7718 changed the title ~~New metric type Listgauge~~ New metric type Listgauge Jun 13, 2018

rayman7718 changed the title ~~New metric type Listgauge~~ New metric type Listgauge JIRA: SAMZA-1733 Jun 13, 2018

rayman7718 changed the title ~~New metric type Listgauge JIRA: SAMZA-1733~~ SAMZA-1733 New metric type Listgauge Jun 14, 2018

asfgit closed this in f6a2d54 Jun 14, 2018


		val taskStoreRestorationMetrics: util.Map[TaskName, Gauge[Long]] = new util.HashMap[TaskName, Gauge[Long]]()

		// A string-gauge metric to capture exceptions at this container



		/**
		* A {@link ListGauge} is a {@link org.apache.samza.metrics.Metric} that buffers multiple instances of a type T in a list.

SAMZA-1733 New metric type Listgauge #541

SAMZA-1733 New metric type Listgauge #541

Uh oh!

Conversation

rayman7718 commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cameronlee314 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!

rayman7718 commented Jun 2, 2018

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!

rayman7718 left a comment

Choose a reason for hiding this comment

Uh oh!

vjagadish1989 commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rayman7718 commented Jun 6, 2018

Uh oh!

vjagadish1989 left a comment

Choose a reason for hiding this comment

rayman7718 commented May 31, 2018 •

edited

Loading

vjagadish1989 commented Jun 5, 2018 •

edited

Loading

vjagadish1989 Jun 7, 2018 •

edited

Loading

vjagadish1989 Jun 7, 2018 •

edited

Loading

vjagadish1989 left a comment •

edited

Loading