Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

(Updated July 22, 2015, 5 p.m.)


Review request for samza.


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs (updated)
-

  samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
8eac8ef 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
 e63d62c 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala, 
> > lines 36-37
> > 
> >
> > though it works, prefer to use the "def" here, not only because it has 
> > leff overhead, but also keep all the methods consistent for better 
> > readability. What do you think?
> 
> Roger Hoover wrote:
> Sounds good.  I only baulked on it the first time because I'm not that 
> skilled with Scala type decarations yet. :)  I can make this work
> 
> Roger Hoover wrote:
> I take it back.  It seems it [can't be 
> done](http://www.scala-lang.org/old/node/5082)

aha, sorry for the misleading. I think, what I mean here is to change the "val" 
to "def": val newCounter = metricGroup.newCounter(_) ==> def newCounter(name: 
String).


- Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---


On July 22, 2015, 4:07 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 22, 2015, 4:07 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

(Updated July 22, 2015, 4:07 a.m.)


Review request for samza.


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs (updated)
-

  samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
8eac8ef 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
 e63d62c 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala, 
> > lines 36-37
> > 
> >
> > though it works, prefer to use the "def" here, not only because it has 
> > leff overhead, but also keep all the methods consistent for better 
> > readability. What do you think?
> 
> Roger Hoover wrote:
> Sounds good.  I only baulked on it the first time because I'm not that 
> skilled with Scala type decarations yet. :)  I can make this work

I take it back.  It seems it [can't be 
done](http://www.scala-lang.org/old/node/5082)


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java, line 23
> > 
> >
> > of the class "the" extends -> of the class "that" extends

Thanks


> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java, line 29
> > 
> >
> > can we also have a constructor with the default prefix ""?

Good idea


> On July 21, 2015, 5:42 p.m., Yan Fang wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala, 
> > lines 36-37
> > 
> >
> > though it works, prefer to use the "def" here, not only because it has 
> > leff overhead, but also keep all the methods consistent for better 
> > readability. What do you think?

Sounds good.  I only baulked on it the first time because I'm not that skilled 
with Scala type decarations yet. :)  I can make this work


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review92429
---



samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java (line 23)


of the class "the" extends -> of the class "that" extends



samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java (line 29)


can we also have a constructor with the default prefix ""?



samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala (line 29)


the extends -> that extends



samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala (lines 
36 - 37)


though it works, prefer to use the "def" here, not only because it has leff 
overhead, but also keep all the methods consistent for better readability. What 
do you think?


- Yan Fang


On July 21, 2015, 5:41 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 21, 2015, 5:41 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
> PRE-CREATION 
>   samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
> 8eac8ef 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
>  PRE-CREATION 
>   
> samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
>  e63d62c 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

(Updated July 21, 2015, 5:41 a.m.)


Review request for samza.


Changes
---

Fixed tests and added base class for Java metrics


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs (updated)
-

  samza-core/src/main/java/org/apache/samza/metrics/MetricGroup.java 
PRE-CREATION 
  samza-core/src/main/java/org/apache/samza/metrics/MetricsBase.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/metrics/MetricsHelper.scala 
8eac8ef 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java
 PRE-CREATION 
  
samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java
 e63d62c 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

(Updated July 15, 2015, 4:45 a.m.)


Review request for samza.


Changes
---

Removed extra space


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs (updated)
-

  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

> On 七月 14, 2015, 9:44 p.m., Yan Fang wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java,
> >  line 24
> > 
> >
> > can this class extends MetricsHelper? This can simplifies a little.
> 
> Roger Hoover wrote:
> I don't see how it simplifies things because I have to implement all the 
> methods in the Scala trait.  I'm having trouble getting the newGauge 
> signatures to match.
> 
> ```
> public class ElasticsearchSystemProducerMetrics implements MetricsHelper {
> public final Counter bulkSendSuccess;
> public final Counter inserts;
> public final Counter updates;
> private final MetricsRegistry registry;
> private final String group;
> private final String systemName;
> 
> public interface JFunction {
> R apply();
> }
> 
> public ElasticsearchSystemProducerMetrics(String systemName, 
> MetricsRegistry registry) {
> group = this.getClass().getName();
> this.registry = registry;
> this.systemName = systemName;
> 
> bulkSendSuccess = newCounter("bulk-send-success");
> inserts = newCounter("docs-inserted");
> updates = newCounter("docs-updated");
> }
> 
> @Override
> public Counter newCounter(String name) {
> return MetricsHelper$class.newCounter(this, name);
> }
> 
> @Override
> public  Gauge newGauge(String name, T value) {
> return MetricsHelper$class.newGauge(this, name, value);
> }
> 
> @Override
> public  Gauge newGauge(String name, JFunction value) {
> return null;
> }
> 
> @Override
> public Timer newTimer(String name) {
> return MetricsHelper$class.newTimer(this, name);
> }
> 
> @Override
> public String getPrefix() {
> return systemName + "-";
> }
> 
> @Override
> public MetricsRegistry registry() {
> return registry;
> }
> 
> @Override
> public String group() {
> return group;
> }
> }
> ```
> 
> Roger Hoover wrote:
> We really only need counters for this class but have to figure out how to 
> implement the Scala newGauge methods which are tricky.  Would appreciate help 
> if you know how to do it.
> 
> Yan Fang wrote:
> Oh, I see. Sorry for the confusion. I did not realize there is a 
> java-scala issue sitting here. Ok. I am fine with going the original 
> approach, which seems clear enough

Another way is to add a Java version of MetricsHelper. Then 
ElasticsearchSystemProducerMetrics and other future java-version metrics can 
extend it. This helps future implementation. Otherwise, I think the first patch 
is fine. Let you make the decision. Thanks. :)


- Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---


On 七月 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated 七月 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

> On July 14, 2015, 9:44 p.m., Yan Fang wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java,
> >  line 24
> > 
> >
> > can this class extends MetricsHelper? This can simplifies a little.
> 
> Roger Hoover wrote:
> I don't see how it simplifies things because I have to implement all the 
> methods in the Scala trait.  I'm having trouble getting the newGauge 
> signatures to match.
> 
> ```
> public class ElasticsearchSystemProducerMetrics implements MetricsHelper {
> public final Counter bulkSendSuccess;
> public final Counter inserts;
> public final Counter updates;
> private final MetricsRegistry registry;
> private final String group;
> private final String systemName;
> 
> public interface JFunction {
> R apply();
> }
> 
> public ElasticsearchSystemProducerMetrics(String systemName, 
> MetricsRegistry registry) {
> group = this.getClass().getName();
> this.registry = registry;
> this.systemName = systemName;
> 
> bulkSendSuccess = newCounter("bulk-send-success");
> inserts = newCounter("docs-inserted");
> updates = newCounter("docs-updated");
> }
> 
> @Override
> public Counter newCounter(String name) {
> return MetricsHelper$class.newCounter(this, name);
> }
> 
> @Override
> public  Gauge newGauge(String name, T value) {
> return MetricsHelper$class.newGauge(this, name, value);
> }
> 
> @Override
> public  Gauge newGauge(String name, JFunction value) {
> return null;
> }
> 
> @Override
> public Timer newTimer(String name) {
> return MetricsHelper$class.newTimer(this, name);
> }
> 
> @Override
> public String getPrefix() {
> return systemName + "-";
> }
> 
> @Override
> public MetricsRegistry registry() {
> return registry;
> }
> 
> @Override
> public String group() {
> return group;
> }
> }
> ```
> 
> Roger Hoover wrote:
> We really only need counters for this class but have to figure out how to 
> implement the Scala newGauge methods which are tricky.  Would appreciate help 
> if you know how to do it.

Oh, I see. Sorry for the confusion. I did not realize there is a java-scala 
issue sitting here. Ok. I am fine with going the original approach, which seems 
clear enough


- Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---


On July 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 14, 2015, 9:44 p.m., Yan Fang wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java,
> >  line 24
> > 
> >
> > can this class extends MetricsHelper? This can simplifies a little.
> 
> Roger Hoover wrote:
> I don't see how it simplifies things because I have to implement all the 
> methods in the Scala trait.  I'm having trouble getting the newGauge 
> signatures to match.
> 
> ```
> public class ElasticsearchSystemProducerMetrics implements MetricsHelper {
> public final Counter bulkSendSuccess;
> public final Counter inserts;
> public final Counter updates;
> private final MetricsRegistry registry;
> private final String group;
> private final String systemName;
> 
> public interface JFunction {
> R apply();
> }
> 
> public ElasticsearchSystemProducerMetrics(String systemName, 
> MetricsRegistry registry) {
> group = this.getClass().getName();
> this.registry = registry;
> this.systemName = systemName;
> 
> bulkSendSuccess = newCounter("bulk-send-success");
> inserts = newCounter("docs-inserted");
> updates = newCounter("docs-updated");
> }
> 
> @Override
> public Counter newCounter(String name) {
> return MetricsHelper$class.newCounter(this, name);
> }
> 
> @Override
> public  Gauge newGauge(String name, T value) {
> return MetricsHelper$class.newGauge(this, name, value);
> }
> 
> @Override
> public  Gauge newGauge(String name, JFunction value) {
> return null;
> }
> 
> @Override
> public Timer newTimer(String name) {
> return MetricsHelper$class.newTimer(this, name);
> }
> 
> @Override
> public String getPrefix() {
> return systemName + "-";
> }
> 
> @Override
> public MetricsRegistry registry() {
> return registry;
> }
> 
> @Override
> public String group() {
> return group;
> }
> }
> ```

We really only need counters for this class but have to figure out how to 
implement the Scala newGauge methods which are tricky.  Would appreciate help 
if you know how to do it.


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---


On July 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 14, 2015, 9:44 p.m., Yan Fang wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 152
> > 
> >
> > remove the space

Sure thing.


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---


On July 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

> On July 14, 2015, 9:44 p.m., Yan Fang wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java,
> >  line 24
> > 
> >
> > can this class extends MetricsHelper? This can simplifies a little.

I don't see how it simplifies things because I have to implement all the 
methods in the Scala trait.  I'm having trouble getting the newGauge signatures 
to match.

```
public class ElasticsearchSystemProducerMetrics implements MetricsHelper {
public final Counter bulkSendSuccess;
public final Counter inserts;
public final Counter updates;
private final MetricsRegistry registry;
private final String group;
private final String systemName;

public interface JFunction {
R apply();
}

public ElasticsearchSystemProducerMetrics(String systemName, 
MetricsRegistry registry) {
group = this.getClass().getName();
this.registry = registry;
this.systemName = systemName;

bulkSendSuccess = newCounter("bulk-send-success");
inserts = newCounter("docs-inserted");
updates = newCounter("docs-updated");
}

@Override
public Counter newCounter(String name) {
return MetricsHelper$class.newCounter(this, name);
}

@Override
public  Gauge newGauge(String name, T value) {
return MetricsHelper$class.newGauge(this, name, value);
}

@Override
public  Gauge newGauge(String name, JFunction value) {
return null;
}

@Override
public Timer newTimer(String name) {
return MetricsHelper$class.newTimer(this, name);
}

@Override
public String getPrefix() {
return systemName + "-";
}

@Override
public MetricsRegistry registry() {
return registry;
}

@Override
public String group() {
return group;
}
}
```


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---


On July 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Re: Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Yan Fang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/#review91670
---



samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 (line 152)


remove the space



samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 (line 24)


can this class extends MetricsHelper? This can simplifies a little.


- Yan Fang


On July 14, 2015, 6:12 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36473/
> ---
> 
> (Updated July 14, 2015, 6:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> SAMZA-733 Add metrics to Elasticsearch System Producer
> 
> 
> Diffs
> -
> 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
>  a277b69 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  7eb14a2 
>   
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36473/diff/
> 
> 
> Testing
> ---
> 
> Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
> stream and that the metrics correctly count how many Elasticsearch documents 
> were created and indexed.
> 
> 
> Thanks,
> 
> Roger Hoover
> 
>

Review Request 36473: SAMZA-733 Add metrics to Elasticsearch System Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36473/
---

Review request for samza.


Repository: samza


Description
---

SAMZA-733 Add metrics to Elasticsearch System Producer


Diffs
-

  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemFactory.java
 a277b69 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 7eb14a2 
  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/36473/diff/


Testing
---

Tested that metrics for Elasticsearch producer appear in JMX and the metrics 
stream and that the metrics correctly count how many Elasticsearch documents 
were created and indexed.


Thanks,

Roger Hoover