[jira] [Commented] (KNOX-940) Support REST access exposing metrics

["Mohammad Kamrul Islam (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KNOX-940?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16035670#comment-16035670 ] 

Mohammad Kamrul Islam commented on KNOX-940:
--------------------------------------------

Thanks [~moresandeep] for detailed review comments. I accepted most of your comment and plan to include in the next revision. Also I added few comments. Can you please review those? After getting your feedback, I will upload the patch accordingly.  


* pom issue: as you found, I was testing against 0.12. It should be 0.13.0 instead. Updating the patch accordingly.

 >>> Can you also briefly describe the overall architecture and the url pattern that can be used to access the metrics, it will be helpful for the review and for future reference.
   
Once the patch is cleared, I will write a doc explaining the details. I used the following two commands for testing, planning to add more such health end points: 

* curl -iv 'https://hadooppoc03-sjc1.prod.uber.internal:8443/gateway/health/v0/metrics?pretty=true'
* curl –iv 'https://hadooppoc03-sjc1.prod.uber.internal:8443/gateway/health/v0/ping'

>>> In class DefaultMetricsService, any specific reason to make 'MetricRegistry metrics' static.

Listener classes needs to access this statically. There could be better way but I followed the pattern defined in dropwizard tutorial (http://metrics.dropwizard.io/3.1.0/manual/servlets/#adminservlet) 

>>> HealthServiceMessages class, the messages are too generic like "Exception occurred" it would be good to have messages which indicate the cause of failure.Method "basicInfo(String original)" boes not appear to be used.

Will be addressed in the new patch.

>>>MetricsResource class:
>>> Path has hard coded version (/v0/metrics), if the version is hard coded do we really need it?

I don’t have any strong opinion here. I came from a perspective that if we want to support backward compatibility in future for REST end-points, we can use version in the API. For each version, we will have a separate Resource file. Each resource could be associated with one version. 
However, if you think it is not needed for Knox, I can take that out. Or you can suggest alternative approach that I can follow. 
 
>>> Using instanceof is probably a bad idea as pointed by Larry McCay recently, it breaks the Open Close Principle and Liskov's Substitution Principle

Good point. I believe most of the doc was primarily  talking about class hierarchy and how instanceOf could be a bad choice. In this instance, I need to cast and I want to verify if cast exception would occur. If you still think, it will be an issue, I’m open for any alternative option.

>>> Why do we have a doPost method when it does not handle POST (or may be I read it wrong)
>>>doGet() method produces JSON as well as XML, as per the method annotations but the response content type is only JSON

I borrowed both patterns that was used for similar Knox services (i.e. knoxtoken service code – TokenResource.java). I don’t need those explicitly but followed the same styles.

>>>in the method getMetrics(), on error the response is Response.ok().entity(String.format("Failed to reply correctly due to : %s ", ioe)).build(), shouldn't it be something like a Response.serverError()..

will be addressed in new patch.

>>>PingResource class
>>>Can you explain the purpose of this class

The intention is to provide hearbeat type of query quickly. Again following the pattern mentioned in dropwizard tutorial (http://metrics.dropwizard.io/3.1.0/manual/servlets/#pingservlet).

>>>Path has hard-coded version (/v0/ping), if the version is hard coded do we really need it ?
>>> Why do we have a doPost method when it does not handle POST (or may be I read it wrong)
>>> in the method getPingResponse(), on error the response is Response.ok().entity(String.format("Failed to reply correctly due to : %s ", ioe)).build(), shouldn't it be something like a Response.serverError().

Replied before.


> Support REST access exposing metrics
> ------------------------------------
>
>                 Key: KNOX-940
>                 URL: https://issues.apache.org/jira/browse/KNOX-940
>             Project: Apache Knox
>          Issue Type: Task
>    Affects Versions: 0.11.0
>            Reporter: Mohammad Kamrul Islam
>            Assignee: Mohammad Kamrul Islam
>             Fix For: 0.13.0
>
>         Attachments: KNOX-940.patch
>
>
> KNOX-643 added the support to publish metrics. However, the metrics are not accessible through REST. This JIRA is to add REST support.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)