Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORM-205. Add REST API to Storm UI. #101

Merged
merged 10 commits into from Jun 4, 2014
Merged

Conversation

harshach
Copy link
Contributor

@harshach harshach commented May 4, 2014

  1. removed html rendering from core.clj
  2. added RESTful apis to core.clj
  3. moved html rendering to jquery & mustache on top of newly added restful apis
  4. added api
    1. /api/cluster/configuration
    2. /api/cluster/summary
    3. /api/supervisor/summary
    4. /api/topology/summary
    5. /api/topology/:id
    6. /api/topology/:id/component/:component
    7. /api/topology/:id/activate (POST)
    8. /api/topology/:id/deactivate (POST)
    9. /api/topology/:id/rebalance/:wait-time (POST)
    10. /api/topology/:id/kill/:wait-time (POST)
      all of the above methods returns json response.

@ptgoetz
Copy link
Member

ptgoetz commented May 6, 2014

I think this is a pretty good start. Switching over to a REST API will enable external systems to do monitoring, visualizations, etc.

However, there are a couple of issues that need to be addressed:

Main Screen:

  • Topology Summary: uptime is not formatted Xm Xs

Topology Detail:

  • Topology stats: # acked is always 0
  • Spouts: Transferred and Acked fields are blank
  • Bolts: Transferred, Execute Latency, and Process Latency are blank
  • Capacity values are off on all screens

In core.clj:

  • URL decoding should use the url-decode function from backtype.storm.util.clj to enforce the use of UTF-8 encoding.

@harshach
Copy link
Contributor Author

harshach commented May 6, 2014

Thanks for the feedback @ptgoetz. I will fix the above mentioned issues.

@@ -48,92 +48,6 @@
(map #(.get_stats ^ExecutorSummary %))
(filter not-nil?)))

(def tips
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the help text is now replicated possibly to several different HTML templates. Is there a way to consolidate these so that changes only need to be made in one place in the future, or is that really difficult to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this since I removed all html rendering from core.clj this made it inconvenient. I will look into mustache templates if it this can be in one place.

@d2r
Copy link

d2r commented May 28, 2014

I get a weird error when trying to compile this:

Caused by: java.security.KeyStoreException: problem accessing trust storejava.io.UTFDataFormatException: malformed input around byte 13

I am not sure why I need to access a java trust store just to compile this code. Any idea what might be wrong?

@harshach
Copy link
Contributor Author

d2r thats werid. I just did a rebase against master and ran the build . It went through fine with all the tests passing

@ptgoetz
Copy link
Member

ptgoetz commented May 28, 2014

I was able to build this as well without issue. I'm testing now in a clustered environment.

@kishorvpatil
Copy link
Contributor

Looks fine to me. +1

@kishorvpatil
Copy link
Contributor

Tested api methods and looks good.

@ptgoetz
Copy link
Member

ptgoetz commented May 28, 2014

+1

At some point in the 0.9.x line a regression was introduced such that the complete latency always displays as 0. I see the same behavior here.

If that can be fixed as part of this pull request, that would be great. If not, then we should create a JIRA for it. I would like to see that fixed for the next release.

@harshach
Copy link
Contributor Author

@ptgoetz I remember fixing that let me check again.

@harshach
Copy link
Contributor Author

@ptgoetz Added complete-latency to the stats. Can you please try it once.
Thanks.

added complete-latencies to the topology-stats
@ptgoetz
Copy link
Member

ptgoetz commented May 29, 2014

+1

@revans2
Copy link
Contributor

revans2 commented May 29, 2014

I agree with d2r that I would like to see the tool tips centralized if possible, I would also like to see a version on the REST apis, something like api/v1/... And it would be nice to have some documentation on the APIs somewhere.

@ptgoetz
Copy link
Member

ptgoetz commented May 29, 2014

@d2r @revans2 Would you guys be okay with opening a JIRA for consolidating the tool tips? As far as documentation, it should probably be added to the docs on the storm website (outside this pull request).

@harshach Can you update the resource paths so the begin with /api/v1/?

@revans2
Copy link
Contributor

revans2 commented May 29, 2014

I am fine with doing documentation and cleanup later.

@d2r
Copy link

d2r commented May 29, 2014

@d2r @revans2 Would you guys be okay with opening a JIRA for consolidating the tool tips?

Sure that is fine.

@ptgoetz
Copy link
Member

ptgoetz commented May 29, 2014

@harshach it looks like this commit reverts the previous commit to fix the complete latency metrics... Can you double check?

undid the complete-latencies fix with last commit added back
@ptgoetz
Copy link
Member

ptgoetz commented May 30, 2014

@revans2 @d2r Are you okay with merging this as-is with a follow up JIRA for cleanup/doc?

@d2r
Copy link

d2r commented May 31, 2014

I get a weird error when trying to compile this:

Never mind. Rebooted and did not see the issue again.

{ "stormVersion" (read-storm-version)
"nimbusUptime" (pretty-uptime-sec (.get_nimbus_uptime_secs summ))
"supervisors" (count sups)
"topologies" ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like topologies is always blank with /api/v1/cluster/summary

@harshach
Copy link
Contributor Author

@d2r Thanks for catching that. Topologies info can be retried from /api/v1/topology/summary . I am not sure about including it as part of cluster/summary. If it makes sense I'll add there otherwise I'll remove the "topologies" element from the response.

removed empty topologies element from cluster-summary.
:title (:spout-failed tips)}}]
(for [k (concat times [":all-time"])
:let [disp ((display-map k) k)]]
[(link-to (if (= k window) {:class "red"} {})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selected time window in the UI no longer appears as red. This could be important since the user would not know whether a window applied to the metrics without looking at the URL.

@d2r
Copy link

d2r commented Jun 2, 2014

It will be great to have an API with JSON responses coming back. I would like to resolve at least the issue with the errors table on the component page, and then I will be +1. The other issues can go in a follow-on JIRA.

@harshach
Copy link
Contributor Author

harshach commented Jun 2, 2014

@d2r component errors page is working fine. I guess its my template naming thats not good. There is div "error" which get update if the API responded with any error and it doesn't return any proper json.
errors-template is part of component-page-templates.html which get updated if there are any errors for the bolts or spouts returned by component api. I'll change the naming to be more appropriate.

@d2r
Copy link

d2r commented Jun 2, 2014

This is the rendered HTML I see:

<table id="errors-table" class="zebra-striped">
    <thead></thead>
    <tbody>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr></tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
        <tr>
            <td></td>
            <td></td>
        </tr>
    </tbody>
</table>

It looks like 9 rows with two blank cells each, and one row without any cells.

I see this on Firefox and Safari on OSX. It seems to know there are errors there, but they are not getting populated for some reason...

@d2r
Copy link

d2r commented Jun 2, 2014

It looks like 9 rows with two blank cells each, and one row without any cells.

Correction: It is 10 rows of two blank cells each.

@harshach
Copy link
Contributor Author

harshach commented Jun 2, 2014

@d2r thanks for the info. I am testing few things I'll update the PR with the fix.

@d2r
Copy link

d2r commented Jun 2, 2014

I know what it is. The errors are doubly-nested in lists. So you have response["errors"][0][0]["time"] to get the time of the first error, instead of response["errors"][0]["time"]. It's noticeable in the JSON output.

@harshach
Copy link
Contributor Author

harshach commented Jun 2, 2014

yeah error is in returning the json here https://github.com/harshach/incubator-storm/blob/ui-rest-new/storm-core/src/clj/backtype/storm/ui/core.clj#L514 . updating PR with fix.

Fixed component errors response. Changed naming of template ids and div.
@harshach
Copy link
Contributor Author

harshach commented Jun 2, 2014

@d2r can you please check the latest update. I've tested on a small cluster here is a screenshot
http://grab.by/xoP4

@d2r
Copy link

d2r commented Jun 3, 2014

+1 that fixed it.
I love how much faster this feels. I like the renames you made too—much clearer.

@revans2
Copy link
Contributor

revans2 commented Jun 4, 2014

looks good to me +1

@asfgit asfgit merged commit 71817e7 into apache:master Jun 4, 2014
@harshach
Copy link
Contributor Author

harshach commented Jun 4, 2014

Thanks for the feedback on this. Working on documentation and other recommendations that made earlier will push those as part of new JIRA.

@pvb05
Copy link

pvb05 commented Jun 17, 2014

Can we get metrics for a supplied time window?

@harshach
Copy link
Contributor Author

@Pradeepbadiger Yes you can pass a window param (in secs) to
/api/v1/topology/:id and /api/v1/topology/:id/component/:component . It only support time windows that UI shows. There is no arbitrary time windows since metrics collection in storm doesn't support that yet.
I am finishing up documentation on rest api that will give more detailed description.

@msukmanowsky
Copy link

First off, thanks very much for this guys, having a RESTful JSON API for Storm is awesome. Just curious, is JSON-P supported by the API endpoints? This would allow users (like myself) to build something like elasticsearch-head and avoid issues with same-origin policy in browsers.

@harshach
Copy link
Contributor Author

@msukmanowsky right now all that api returns is a json response. Could you please file a jira for jsonp support.

@msukmanowsky
Copy link

@Gvain
Copy link
Contributor

Gvain commented Jul 2, 2014

@harshach Strange, i don't see any spout-stats section or bolt-stats-section in topology page. And thus i can't follow into component page or logviewer page. Am i missing something ?

@harshach
Copy link
Contributor Author

harshach commented Jul 2, 2014

@Gvain I just downloaded apache-storm-0.9.2-incubating and ran a wordcount topology. Here is my UI
http://grab.by/yg8y
http://grab.by/yg8u
http://grab.by/yg8q.
Can you tell me which browser you are using. Also check if its not related to
https://issues.apache.org/jira/browse/STORM-370

@harshach
Copy link
Contributor Author

harshach commented Jul 2, 2014

@Gvain Incase if you not already done it can you clear the browser cache.

@HeartSaVioR
Copy link
Contributor

There seems to javascript syntax error : jquery.tablesorter.min.js
There're some related issues and post on mailing list, and actually I've pull a request to fix it.

https://issues.apache.org/jira/browse/STORM-381 and #174

@Gvain
Copy link
Contributor

Gvain commented Jul 3, 2014

@harshach I am using chrome, and latest storm from master branch, so, STORM-370 has already merged in. it should not related to it as my problem page is topology page while STORM-370 is component page. I also cleared my chrome cache, it doesn't help. My ui topology page looks like this:

image

@shell0dh
Copy link
Contributor

shell0dh commented Jul 3, 2014

jquery.tablesorter.min.js issue.
Set your browser encoding utf-8
or all *.html add ""

@Gvain
Copy link
Contributor

Gvain commented Jul 3, 2014

@shell0dh Yeap, that's it Thanks a lot.

@shell0dh
Copy link
Contributor

shell0dh commented Jul 3, 2014

@Gvain
When there is an exception, the exception time is formatted with multiple languages. Local language will be replaced by question marks.
You can apply #171 solve the problem.

@Gvain
Copy link
Contributor

Gvain commented Jul 4, 2014

@shell0dh , Thanks for your remind. I will try it.

knusbaum pushed a commit to knusbaum/incubator-storm that referenced this pull request Feb 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet