Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 18, 2016, 10:18 p.m.)


Review request for samza and Jake Maes.


Repository: samza


Description (updated)
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs. 
 Updated diff = 
Adding monitorName in MonitorFactory getMonitor method to enable creation of 
namespaced metrics from each of the monitor. monitorName can be used by 
MonitorFactory implementations in the metrics that are created and reported 
from them. 
Jira associated with the patch : 
https://issues.apache.org/jira/browse/SAMZA-1049


Diffs
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/monitor/MonitorFactory.java 
c38a5d8f737ee5cf39a4876697025fa9cd300bff 
  samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java 
dcf9e5734b1d97e37e4fd0e89cc7f525a37a0d65 
  samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 
754ad82af9ef0f2334ec629160be39d183df1b38 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 
e75f4946f38523284921798578f2a3db9c02f599 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorFactory.java 
ccc534efc8594837f83b9126ca3d4ee22d8fbb05 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java
 af414b46dc5c2ff1579deb2c247cd94388a0b3ea 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java 
5b19001eb5db51b6cc4440cf74be92f285ab0209 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jake Maes]

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


Ship it!




I'll try commiting it once the patch is posted on a JIRA

- Jake Maes


On Nov. 16, 2016, 5:44 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 16, 2016, 5:44 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs. 
>  Updated diff = 
> Adding monitorName in MonitorFactory getMonitor method to enable creation of 
> namespaced metrics from each of the monitor. monitorName can be used by 
> MonitorFactory implementations in the metrics that are created and reported 
> from them.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/monitor/MonitorFactory.java 
> c38a5d8f737ee5cf39a4876697025fa9cd300bff 
>   samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java 
> dcf9e5734b1d97e37e4fd0e89cc7f525a37a0d65 
>   samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 
> 754ad82af9ef0f2334ec629160be39d183df1b38 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 
> e75f4946f38523284921798578f2a3db9c02f599 
>   
> samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorFactory.java
>  ccc534efc8594837f83b9126ca3d4ee22d8fbb05 
>   
> samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java
>  af414b46dc5c2ff1579deb2c247cd94388a0b3ea 
>   
> samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java
>  5b19001eb5db51b6cc4440cf74be92f285ab0209 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 16, 2016, 5:44 a.m.)


Review request for samza and Jake Maes.


Repository: samza


Description (updated)
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs. 
 Updated diff = 
Adding monitorName in MonitorFactory getMonitor method to enable creation of 
namespaced metrics from each of the monitor. monitorName can be used by 
MonitorFactory implementations in the metrics that are created and reported 
from them.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/monitor/MonitorFactory.java 
c38a5d8f737ee5cf39a4876697025fa9cd300bff 
  samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java 
dcf9e5734b1d97e37e4fd0e89cc7f525a37a0d65 
  samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java 
754ad82af9ef0f2334ec629160be39d183df1b38 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java 
e75f4946f38523284921798578f2a3db9c02f599 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitorFactory.java 
ccc534efc8594837f83b9126ca3d4ee22d8fbb05 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitorFactory.java
 af414b46dc5c2ff1579deb2c247cd94388a0b3ea 
  
samza-rest/src/test/java/org/apache/samza/monitor/mock/MockMonitorFactory.java 
5b19001eb5db51b6cc4440cf74be92f285ab0209 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Fred Ji]

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


Ship it!




Ship It!

- Fred Ji


On Nov. 11, 2016, 12:22 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 11, 2016, 12:22 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jake Maes]

> On Nov. 9, 2016, 11:18 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > What's the execution unit here? The samza-rest server? The monitor? The 
> > container being monitored?
> 
> Shanthoosh Venkataraman wrote:
> I think `execution unit` is a misnomer. By execution unit, I meant the 
> origin of the metrics (physical hostname in which the samza rest process is 
> running/SamzaRestMonitor literal/SamzaRestResource literal etc)
> 
> Shanthoosh Venkataraman wrote:
> Discussed offline. Exposing containerName through samza-rest config might 
> be unnecessary as of now. Hardcoding it to hostname on which `samza-rest` is 
> running.

I really think you should experiment with this because I suspect the host name 
will be redundant. Metric reporter implementatations will commonly report the 
host name if applicable, so it's unnecessary for the metric name to include the 
host. 

Please try it out with AMF at LinkedIn and report back with the metric 
structure, so we can be sure it makes sense.


- Jake


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


On Nov. 11, 2016, 12:22 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 11, 2016, 12:22 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jagadish Venkatraman]

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


Fix it, then Ship it!




After addressing the nit. Thanks!


docs/learn/documentation/versioned/rest/monitors.md (line 96)


nit: Nuke new lines. I think the 3 lines can be a single paragraph.


- Jagadish Venkatraman


On Nov. 11, 2016, 12:22 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 11, 2016, 12:22 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Prateek Maheshwari]

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


Ship it!




Ship It!

- Prateek Maheshwari


On Nov. 10, 2016, 4:22 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 10, 2016, 4:22 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 11, 2016, 12:22 a.m.)


Review request for samza and Jake Maes.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 75
> > 
> >
> > I don't think the MetricsConfig constructure takes a subset. 
> > 
> > I think it takes the root and expects to find the "metrics" prefix
> 
> Jake Maes wrote:
> s/constructure/constructor
> 
> phonetic brain fail
> 
> Shanthoosh Venkataraman wrote:
> Yes, that's true. It expects the root and finds the metrics prefix. 
> Hence, stripPrefix is passed on as false, so that prefix isn't removed. This 
> just selects the config subset that starts with METRICS_PREFIX, without 
> removing the prefix. The goal is to not pass on the entire config object and 
> pass only metrics related config into MetricsConfig constructor.
> 
> Prateek Maheshwari wrote:
> Regarding the goal: That's not how the XConfig classes are intended to be 
> used, so it's not really necessary to do this. For example, see the implicit 
> conversion b/w Config and XConfig. Prefer passing in config.

Done.


- Shanthoosh


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


On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 10, 2016, 1:04 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 27
> > 
> >
> > Unused import.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 164
> > 
> >
> > debug?

I think this should be info. It will be useful to know the registered reporters 
while debugging.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, 
> > line 39
> > 
> >
> > I meant remove the whole javadoc including @params. There's no useful 
> > additional information here.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 107
> > 
> >
> > Should this be called during start()? Same for stop().

To accomplish this, both(SamzaRestService,SamzaMonitorService) of them had to 
be coupled. Logically they are seperate entities. One responsible for resources 
and other monitors. Currently MonitorService & RestService are not coupled with 
each other. It would be useful keep them seperate(In the future, we could have 
SamzaRestService and SamzaMonitorService deployed seperately when we have too 
many(Monitors & Resources) in them). I would prefer not to have this coupling.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala,
> >  line 56
> > 
> >
> > Maybe use JavaCoverters._ and .asScala?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala,
> >  line 46
> > 
> >
> > Maybe use JavaCoverters._ and .asScala?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, 
> > line 30
> > 
> >
> > Remove "contains reusable methods which" and "based upon 
> > configuration". Don't mind removing the whole comment either.

Removed.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 103
> > 
> >
> > Don't understand this interface. What's a SchedulingProvider, and how 
> > is it different than a ScheduledExecutorService?
> > 
> > schedulingProvider should be probably be stopped in stop() in this 
> > class and not in SamzaMonitorService since this class owns it. Either that, 
> > or move instantiation to SamzaMonitorService too.

SchedulingProvider interface looks similar to that of ExecutorService. Not sure 
of the value add this interface provides. I guess, this is not required and we 
could just use ExeuctorService. Added a jira for it, since it's not in the 
scope of this patch. We could re-visit it later.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 166
> > 
> >
> > debug?

I think this should be info. Will be useful to know this when debugging.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 98
> > 
> >
> > Maybe: "are the same as that of Samza jobs." and make "that of Samza 
> > jobs" the configuration link and remove next line?

Done.


> On Nov. 10, 2016, 6:57 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 91
> > 
> >
> > See previous comment about just passing the entire config. Does that 
> > not work?

Done.


- Shanthoosh


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


On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. 

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Prateek Maheshwari]

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



Looks pretty good to me. Some final minor comments and questions.


docs/learn/documentation/versioned/rest/monitors.md (line 98)


Maybe: "are the same as that of Samza jobs." and make "that of Samza jobs" 
the configuration link and remove next line?



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
30)


Remove "contains reusable methods which" and "based upon configuration". 
Don't mind removing the whole comment either.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
39)


I meant remove the whole javadoc including @params. There's no useful 
additional information here.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
52)


Minor, your call: Not a big fan of the "hanging towers of parameters" code 
style. Maybe extract variable?



samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 (line 45)


Maybe use JavaCoverters._ and .asScala?



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 27)


Unused import.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 91)


See previous comment about just passing the entire config. Does that not 
work?



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 103)


Don't understand this interface. What's a SchedulingProvider, and how is it 
different than a ScheduledExecutorService?

schedulingProvider should be probably be stopped in stop() in this class 
and not in SamzaMonitorService since this class owns it. Either that, or move 
instantiation to SamzaMonitorService too.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 107)


Should this be called during start()? Same for stop().



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 164)


debug?



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 166)


debug?



samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
(line 46)


Maybe use JavaCoverters._ and .asScala?


- Prateek Maheshwari


On Nov. 9, 2016, 5:04 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 5:04 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 11:18 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > What's the execution unit here? The samza-rest server? The monitor? The 
> > container being monitored?
> 
> Shanthoosh Venkataraman wrote:
> I think `execution unit` is a misnomer. By execution unit, I meant the 
> origin of the metrics (physical hostname in which the samza rest process is 
> running/SamzaRestMonitor literal/SamzaRestResource literal etc)

Discussed offline. Exposing containerName through samza-rest config might be 
unnecessary as of now. Hardcoding it to hostname on which `samza-rest` is 
running.


- Shanthoosh


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


On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 10, 2016, 1:04 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > How exactly is this used when metrics are reported? 
> > 
> > Is there a reason for this to be configurable / adding a new config 
> > key? 
> > 
> > If it's configurable, we should document it somewhere?
> 
> Shanthoosh Venkataraman wrote:
> Container name here refers to the execution unit from which the metrics 
> gets reported. It's used when instantiating the MetricsReporter. If this 
> configuration is undefined, it's defaulted to hostName on which SamzaRest is 
> running. Resources will report metrics alongside monitors. Added relevent 
> documentation for it.
> 
> Jagadish Venkatraman wrote:
> Is there a use case where I would want to configure a different container 
> name? If not, this need not be a config? (we can default it to `samza-rest`)
> 
> Shanthoosh Venkataraman wrote:
> For LI use case, using hostname(or SamzaRest) would suffice.
> My understanding is that the containerName denotes either the logical 
> host or the physical host or some unique tag to identify the origin from 
> which the metrics gets reported from. 
> This container name could be appended into metrics reported depending 
> upon MetricsReporter implementations. 
> Hence, I think it will be useful to let users configure it. 
> For instance, some implementations could generate a unique string as 
> container name based upon environment and populate it in the config to 
> uniquely identify the environment it's reporting the metrics from.

Discussed offline. Exposing containerName through samza-rest config might be 
unnecessary as of now. Hardcoding it to hostname on which `samza-rest` is 
running.


- Shanthoosh


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


On Nov. 10, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 10, 2016, 1:04 a.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 10, 2016, 1:04 a.m.)


Review request for samza and Jake Maes.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
47b0663637f6db187d86961377ee3ee203b73fdb 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 10, 2016, 12:04 a.m.)


Review request for samza and Jake Maes.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  docs/learn/documentation/versioned/rest/overview.md 
c382f032843cce696a445ff110e87a8198cc96d7 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
47b0663637f6db187d86961377ee3ee203b73fdb 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 11:18 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > What's the execution unit here? The samza-rest server? The monitor? The 
> > container being monitored?

I think `execution unit` is a misnomer. By execution unit, I meant the origin 
of the metrics (physical hostname in which the samza rest process is 
running/SamzaRestMonitor literal/SamzaRestResource literal etc)


- Shanthoosh


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


On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 10:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > How exactly is this used when metrics are reported? 
> > 
> > Is there a reason for this to be configurable / adding a new config 
> > key? 
> > 
> > If it's configurable, we should document it somewhere?
> 
> Shanthoosh Venkataraman wrote:
> Container name here refers to the execution unit from which the metrics 
> gets reported. It's used when instantiating the MetricsReporter. If this 
> configuration is undefined, it's defaulted to hostName on which SamzaRest is 
> running. Resources will report metrics alongside monitors. Added relevent 
> documentation for it.
> 
> Jagadish Venkatraman wrote:
> Is there a use case where I would want to configure a different container 
> name? If not, this need not be a config? (we can default it to `samza-rest`)

For LI use case, using hostname(or SamzaRest) would suffice.
My understanding is that the containerName denotes either the logical host or 
the physical host or some unique tag to identify the origin from which the 
metrics gets reported from. 
This container name could be appended into metrics reported depending upon 
MetricsReporter implementations. 
Hence, I think it will be useful to let users configure it. 
For instance, some implementations could generate a unique string as container 
name based upon environment and populate it in the config to uniquely identify 
the environment it's reporting the metrics from.


- Shanthoosh


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


On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 10:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Prateek Maheshwari]

> On Nov. 5, 2016, 11:47 a.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 75
> > 
> >
> > I don't think the MetricsConfig constructure takes a subset. 
> > 
> > I think it takes the root and expects to find the "metrics" prefix
> 
> Jake Maes wrote:
> s/constructure/constructor
> 
> phonetic brain fail
> 
> Shanthoosh Venkataraman wrote:
> Yes, that's true. It expects the root and finds the metrics prefix. 
> Hence, stripPrefix is passed on as false, so that prefix isn't removed. This 
> just selects the config subset that starts with METRICS_PREFIX, without 
> removing the prefix. The goal is to not pass on the entire config object and 
> pass only metrics related config into MetricsConfig constructor.

Regarding the goal: That's not how the XConfig classes are intended to be used, 
so it's not really necessary to do this. For example, see the implicit 
conversion b/w Config and XConfig. Prefer passing in config.


- Prateek


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


On Nov. 9, 2016, 2:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 2:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jagadish Venkatraman]

> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > How exactly is this used when metrics are reported? 
> > 
> > Is there a reason for this to be configurable / adding a new config 
> > key? 
> > 
> > If it's configurable, we should document it somewhere?
> 
> Shanthoosh Venkataraman wrote:
> Container name here refers to the execution unit from which the metrics 
> gets reported. It's used when instantiating the MetricsReporter. If this 
> configuration is undefined, it's defaulted to hostName on which SamzaRest is 
> running. Resources will report metrics alongside monitors. Added relevent 
> documentation for it.

Is there a use case where I would want to configure a different container name? 
If not, this need not be a config? (we can default it to `samza-rest`)


- Jagadish


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


On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 10:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 65
> > 
> >
> > Why is this taking in a concrete class - ReadableMetricsRegistry ? 
> > Prefer to have this `MetricsRegistry` instead.
> > 
> > We should operate at the level of interfaces whereever possible.

ReadableMetricsRegistry is an interface, not a concrete class. This particular 
type is required for registering with MetricsReporter instances.


> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 95
> > 
> >
> > Documentation looks different (than the rest of the page). Would be 
> > nice if this was instructional in nature.
> > 
> > /s/create/create and report.
> > /s/theirs/yours

Done.


> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 99
> > 
> >
> > 1. s/Please refer/You can refer
> > 2. `custom` is probably redundant.
> > 3. The link and anchor text placement is weird. 
> > 4. Prefer to be precise with documentation. (For example, this line has 
> > the word `metrics reporters` thrice)

Done.


> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 100
> > 
> >
> > This line is not needed IMO. (If we choose to retain, it, should 
> > atleast be re-worded)

Removed.


> On Nov. 9, 2016, 12:09 a.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > How exactly is this used when metrics are reported? 
> > 
> > Is there a reason for this to be configurable / adding a new config 
> > key? 
> > 
> > If it's configurable, we should document it somewhere?

Container name here refers to the execution unit from which the metrics gets 
reported. It's used when instantiating the MetricsReporter. If this 
configuration is undefined, it's defaulted to hostName on which SamzaRest is 
running. Resources will report metrics alongside monitors. Added relevent 
documentation for it.


- Shanthoosh


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


On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 10:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 9, 2016, 10:50 p.m.)


Review request for samza and Jake Maes.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  docs/learn/documentation/versioned/rest/overview.md 
c382f032843cce696a445ff110e87a8198cc96d7 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
47b0663637f6db187d86961377ee3ee203b73fdb 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, 
> > line 36
> > 
> >
> > Minor: private constructors for helper classes are pretty universal, 
> > don't think they need a comment.

Done.


> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/monitors.md, line 100
> > 
> >
> > This section could be more concise.

Changed, removed some of the docs.


> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, 
> > line 40
> > 
> >
> > This javadoc doesn't add any more information than the method signature 
> > already provides. Prefer removing.

Removed.


> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java, line 55
> > 
> >
> > What is this config, and what does container refer to here? The 
> > samza-rest service container?

Container name here refers to the execution unit from which the metrics gets 
reported. It's used when instantiating the MetricsReporter. If this 
configuration is undefined, it's defaulted to hostName on which SamzaRest is 
running. Resources will report metrics alongside monitors.  Added relevent 
documentation for it.


> On Nov. 9, 2016, 12:31 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java, 
> > line 45
> > 
> >
> > Not sure this specific method (for classloading this particular class 
> > from configs) deserves a new Util class. This is a general pattern used in 
> > other places, maybe we should extract that as a util instead.
> > 
> > May be worth revisiting if/how we want to share code b/w samza-rest and 
> > samza-core. Ideally samza-rest shouldn't depend on samza-core, and shared 
> > stuff should be pulled out to samza-api or samza-common (which doesn't 
> > exist yet).

Discussed offline. Extracting utils(and other common functionalities) shared 
between samza-core & samza-rest into samza-common is a longer term goal which 
is required to remove dependencies. However, for the scope of this patch, doing 
this will be a overkill. Punted for now.


- Shanthoosh


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


On Nov. 9, 2016, 10:50 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 9, 2016, 10:50 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   docs/learn/documentation/versioned/rest/overview.md 
> c382f032843cce696a445ff110e87a8198cc96d7 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Prateek Maheshwari]

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




docs/learn/documentation/versioned/rest/monitors.md (line 100)


This section could be more concise.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
36)


Minor: private constructors for helper classes are pretty universal, don't 
think they need a comment.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
40)


This javadoc doesn't add any more information than the method signature 
already provides. Prefer removing.



samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java (line 
45)


Not sure this specific method (for classloading this particular class from 
configs) deserves a new Util class. This is a general pattern used in other 
places, maybe we should extract that as a util instead.

May be worth revisiting if/how we want to share code b/w samza-rest and 
samza-core. Ideally samza-rest shouldn't depend on samza-core, and shared stuff 
should be pulled out to samza-api or samza-common (which doesn't exist yet).



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 55)


What is this config, and what does container refer to here? The samza-rest 
service container?


- Prateek Maheshwari


On Nov. 8, 2016, 3:13 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 8, 2016, 3:13 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jagadish Venkatraman]

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




docs/learn/documentation/versioned/rest/monitors.md (line 95)


Documentation looks different (than the rest of the page). Would be nice if 
this was instructional in nature.

/s/create/create and report.
/s/theirs/yours



docs/learn/documentation/versioned/rest/monitors.md (line 99)


1. s/Please refer/You can refer
2. `custom` is probably redundant.
3. The link and anchor text placement is weird. 
4. Prefer to be precise with documentation. (For example, this line has the 
word `metrics reporters` thrice)



docs/learn/documentation/versioned/rest/monitors.md (line 100)


This line is not needed IMO. (If we choose to retain, it, should atleast be 
re-worded)



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java (line 55)


How exactly is this used when metrics are reported? 

Is there a reason for this to be configurable / adding a new config key? 

If it's configurable, we should document it somewhere?



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 65)


Why is this taking in a concrete class - ReadableMetricsRegistry ? Prefer 
to have this `MetricsRegistry` instead.

We should operate at the level of interfaces whereever possible.


- Jagadish Venkatraman


On Nov. 8, 2016, 11:13 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 8, 2016, 11:13 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

(Updated Nov. 8, 2016, 11:13 p.m.)


Review request for samza and Jake Maes.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs (updated)
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
47b0663637f6db187d86961377ee3ee203b73fdb 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-core/src/main/scala/org/apache/samza/util/Util.scala, line 336
> > 
> >
> > This edit looks like a mistake.
> > 
> > Did this file need to be modified at all?

Idea was showing error because of that tag. It's not required for this patch. 
Removed.


> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 73
> > 
> >
> > What's the purpose of this static factory?
> > 
> > The typical reason is to construct the object differently (e.g. a 
> > different subclass) based on some parameter. 
> > 
> > I don't see any value of the approach here.
> > 
> > If there is some value, then the constructor should be made private.

Discussed offline. It was harder to test SamzaRestService with all new operator 
instantiations happening in the constructor. Hence, mocking out these 
dependencies were harder. Also with this approach, looking at the constructor 
we get to know the dependencies of SamzaRestService explicitly. However, since 
there are no use cases which would require this factory, removing this factory. 
Keeping the constructor as it is, moving the instantiation into the main method.


> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 75
> > 
> >
> > I don't think the MetricsConfig constructure takes a subset. 
> > 
> > I think it takes the root and expects to find the "metrics" prefix
> 
> Jake Maes wrote:
> s/constructure/constructor
> 
> phonetic brain fail

Yes, that's true. It expects the root and finds the metrics prefix. Hence, 
stripPrefix is passed on as false, so that prefix isn't removed. This just 
selects the config subset that starts with METRICS_PREFIX, without removing the 
prefix. The goal is to not pass on the entire config object and pass only 
metrics related config into MetricsConfig constructor.


> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala,
> >  line 40
> > 
> >
> > We're moving away from Scala. All new files should be Java.

Done. Migrated from scala to java.


> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 60
> > 
> >
> > The Resources will eventually emit metrics too, so I think this value 
> > is too specific.

Changed it to SamzaRest. Not sure of the implications of using a proper name 
here. For instance, in SamzaContainerMetrics source string is assigned to value 
"unknown". I'm most certain that this string is a placeholder to register 
MetricsRegistry instances with MetricsReporter and not used when reporting the 
actual metrics.


- Shanthoosh


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


On Nov. 8, 2016, 11:13 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Nov. 8, 2016, 11:13 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/java/org/apache/samza/util/MetricsReporterLoader.java 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   samza-rest/src/test/java/org/apache/samza/rest/TestSamzaRestService.java 
> PRE-CREATION 
>   
> 

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jake Maes]

> On Nov. 5, 2016, 6:47 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java, line 
> > 75
> > 
> >
> > I don't think the MetricsConfig constructure takes a subset. 
> > 
> > I think it takes the root and expects to find the "metrics" prefix

s/constructure/constructor

phonetic brain fail


- Jake


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


On Oct. 31, 2016, 9:18 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Oct. 31, 2016, 9:18 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/util/Util.scala 
> c4836f202f7eda1d4e71eac94fd48e46207b0316 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestSamzaRestService.java
>  PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Re: Review Request 53297: Initial version of adding metrics into samza rest.

[Jake Maes]

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




samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala 
(line 40)


We're moving away from Scala. All new files should be Java.



samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 335)


This edit looks like a mistake.

Did this file need to be modified at all?



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 60)


The Resources will eventually emit metrics too, so I think this value is 
too specific.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 73)


What's the purpose of this static factory?

The typical reason is to construct the object differently (e.g. a different 
subclass) based on some parameter. 

I don't see any value of the approach here.

If there is some value, then the constructor should be made private.



samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java (line 75)


I don't think the MetricsConfig constructure takes a subset. 

I think it takes the root and expects to find the "metrics" prefix


- Jake Maes


On Oct. 31, 2016, 9:18 p.m., Shanthoosh Venkataraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53297/
> ---
> 
> (Updated Oct. 31, 2016, 9:18 p.m.)
> 
> 
> Review request for samza and Jake Maes.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> This patch aims at enabling users to define custom reporters to send metrics 
> from the monitors. Configurations required for the definition of the metrics 
> reporters follows the same convention as of the samza jobs.
> 
> 
> Diffs
> -
> 
>   docs/learn/documentation/versioned/rest/monitors.md 
> 46678bbe5fed99f767c3324dc9578ee1a64cec66 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
> e0468ee89c89fd720834461771ebb36475475bcb 
>   
> samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
>  f24beb1e099dd44b15b475e0a4a7f70560c6965e 
>   samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala 
> PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/util/Util.scala 
> c4836f202f7eda1d4e71eac94fd48e46207b0316 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
> 47b0663637f6db187d86961377ee3ee203b73fdb 
>   samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
> 2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
>   
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestSamzaRestService.java
>  PRE-CREATION 
>   
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala
>  8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 
> 
> Diff: https://reviews.apache.org/r/53297/diff/
> 
> 
> Testing
> ---
> 
> Unit tests are done to verify the intended functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>

Review Request 53297: Initial version of adding metrics into samza rest.

[Shanthoosh Venkataraman]

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

Review request for samza.


Repository: samza


Description
---

This patch aims at enabling users to define custom reporters to send metrics 
from the monitors. Configurations required for the definition of the metrics 
reporters follows the same convention as of the samza jobs.


Diffs
-

  docs/learn/documentation/versioned/rest/monitors.md 
46678bbe5fed99f767c3324dc9578ee1a64cec66 
  samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 
e0468ee89c89fd720834461771ebb36475475bcb 
  
samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala
 f24beb1e099dd44b15b475e0a4a7f70560c6965e 
  samza-core/src/main/scala/org/apache/samza/util/MetricsReporterLoader.scala 
PRE-CREATION 
  samza-core/src/main/scala/org/apache/samza/util/Util.scala 
c4836f202f7eda1d4e71eac94fd48e46207b0316 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java 
47b0663637f6db187d86961377ee3ee203b73fdb 
  samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java 
2a3e83a24a5343bb53b93fc9d0a647c1b253714b 
  
samza-rest/src/test/java/org/apache/samza/rest/resources/TestSamzaRestService.java
 PRE-CREATION 
  
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala 
8a5b4aaea6e11a5af999f12d50e5b6135dbc70ca 

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


Testing
---

Unit tests are done to verify the intended functionality.


Thanks,

Shanthoosh Venkataraman