new: dev: add implementation & test to verify providing metrics

[vrajat]

A flag to report is definitely required but that is out of scope of this PR. A flag for instrumentation is a good idea but I am concerned that there will be too many if conditions all over the code.

On Wed, May 2, 2018 at 11:56 AM Abhishek Das notifications@github.com wrote:

@abhishekdas99 commented on this pull request.

In rubix-bookkeeper/src/main/java/com/qubole/rubix/bookkeeper/BookKeeperServer.java https://github.com/qubole/rubix/pull/120#discussion_r185402794:

@@ -38,6 +39,9 @@ public static BookKeeper bookKeeper; public static BookKeeperService.Processor processor;

• // Registry for gathering & storing necessary metrics
• private static MetricRegistry metrics = new MetricRegistry();

Should we also have a flag to enable metrics ? Just an implementation detail so that we can turn metrics off if anything goes wrong

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/qubole/rubix/pull/120#discussion_r185402794, or mute the thread https://github.com/notifications/unsubscribe-auth/ABj_mufehaGfviaeaCdaEUuoRXgzw06jks5tuVGMgaJpZM4TrK7n .

[vrajat]

@jordanw-bq Can you also list out how you have tested it beyond unit tests ? We have to make sure this does not break anything during run time. Tests are required on both Qubole clusters (P0) and EMR Presto (P1) but we can punt on EMR for now.

[coveralls]

Pull Request Test Coverage Report for Build 78

• 0 of 7 (0.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.06%) to 22.248%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rubix-bookkeeper/src/main/java/com/qubole/rubix/bookkeeper/BookKeeperServer.java	0	2	0.0%
rubix-bookkeeper/src/main/java/com/qubole/rubix/bookkeeper/BookKeeper.java	0	5	0.0%
<!--	Total:	0	7	0.0%	-->

Totals
Change from base Build 77:	-0.06%
Covered Lines:	475
Relevant Lines:	2135

---

💛 - Coveralls

[vrajat]

@jordanw-bq I have approved the changes. Code looks good. Please make sure you test it on a live Presto cluster due to the dependency changes and also get approval from @abhishekdas99 or @stagraqubole .

[jordanw-bq]

@vrajat @abhishekdas99 These dependency changes have been successfully tested on a Qubole cluster.

[vrajat]

I am going to merge this one. There is a slight drop in coverage and that can be ignored.