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

nsqadmin: refactoring #323

Merged
merged 18 commits into from
Aug 7, 2015
Merged

Conversation

mreiferson
Copy link
Member

nsqadmin is a Go web app, which has some tradeoffs. It's great because it's dead simple to deploy. It's terrible because it's a PITA writing Go based web apps.

I think an ideal middle ground is:

  1. nsqadmin becomes a JSON API with convenience endpoints to query the cluster and aggregate data
  2. nsqadmin UI is a client-side app
  3. all the static assets for (2) are bundled into the binary so it's still easy to deploy (this is already the case, just stating the obvious)

Simultaneously, it could use a facelift, but this refactoring alone makes things much easier to extend and for others to contribute.

cc @visionmedia

@mreiferson
Copy link
Member Author

ping #322

@michaelhood
Copy link
Contributor

really like the client side UI approach. will be exciting to see what people create on it.

@tj
Copy link
Contributor

tj commented Apr 10, 2014

@mreiferson the "messages" count includes messages in-flight right? would you be interested in changing that to only messages that have been FIN'd? or maybe adding another

@mreiferson
Copy link
Member Author

@visionmedia there should be a separate counter for just FIN message count, the "regular" message count is specifically the # of PUB messages to that nsqd

@mreiferson
Copy link
Member Author

edit: s/should be/is

@mreiferson
Copy link
Member Author

edit again: I'm crazy, there isn't...

@mreiferson
Copy link
Member Author

I suppose it is derivable as message_count - in_flight_count (and optionally subtracting deferred_count and requeued_count if you wanted to exclude retried duplicates and count "unique" FIN)

@mreiferson
Copy link
Member Author

@visionmedia you starting to work on this?

@tj
Copy link
Contributor

tj commented Apr 10, 2014

nope just debugging some prod stuff ATM

@pkieltyka
Copy link

Great idea, what about having the admin endpoints on nsqd itself? .. and that way the client-side admin could be just some static html/js files, in the index.html just specify a jsonp endpoint to an nsqlookupd and done.

@mreiferson
Copy link
Member Author

@pkieltyka that's a little more difficult, in short, because that means all of the daemons are exposed to the internet. Technically nsqd and nsqlookupd already do have all the endpoints they need. nsqadmin really just aggregates and coordinates requests.

I think it still makes sense for nsqadmin to serve that role behind the client-side application (and to serve the initial static asset payload).

@pkieltyka
Copy link

That makes sense

@mreiferson mreiferson force-pushed the nsqadmin_importable_323 branch 2 times, most recently from 03d926a to 7255221 Compare September 9, 2014 19:41
@mreiferson mreiferson force-pushed the nsqadmin_importable_323 branch from e500795 to 6e9af9f Compare September 15, 2014 15:29
@allgeek
Copy link

allgeek commented Oct 27, 2014

Is there a timeline for getting this reviewed & landed in master? I'm looking into submitting a pull request for #347, and obviously there are a number of nsqadmin changes involved there. Is there something a new contributor could possibly help with here? (I noticed a Travis build failure on these commits, but haven't dug into it yet - don't want to step on any toes or duplicate efforts)

@mreiferson
Copy link
Member Author

Hi @allgeek - great to hear that you're interested in contributing, thanks!

I just need to finish up here, so this is on me. I've been distracted the past few weeks 😁

If you want to dig into this PR via code review, by all means that would be helpful - this way we're on the same page with the direction I'm taking things.

@mreiferson
Copy link
Member Author

I just remembered - if you are interested in helping out with this PR, I could use some help with the client side changes referenced in the OP.

@allgeek
Copy link

allgeek commented Oct 28, 2014

I'll try to catch you in the IRC channel at some point, although I've got meetings most of the day. Is there a particular JS framework or approach you had wanted to take with the new admin client page? I see the current site just uses jQuery and Bootstrap...

@mreiferson
Copy link
Member Author

some bugs on the Counter page (it's not working correctly for me). Not sure what's happening there yet.

I didn't translate the JS 1:1 here, it's just broken or?

@jehiah
Copy link
Member

jehiah commented Aug 6, 2015

For the counter page, i'm getting Zero's back from the /api/counter endpoint, but also the graphite URLS have unexpected &amp instead of just &.

@jehiah jehiah force-pushed the nsqadmin_importable_323 branch from 11951cf to f9cdd4f Compare August 6, 2015 17:53
@jehiah
Copy link
Member

jehiah commented Aug 6, 2015

I think i fixed all the previously mentioned bugs (please take a look before i squash those); glyphicon fonts are not being embedded properly and i think that's the only outstanding thing left.

@mreiferson
Copy link
Member Author

For the counter page, i'm getting Zero's back from the /api/counter endpoint, but also the graphite URLS have unexpected &amp instead of just &.

Did you figure this out?

@jehiah
Copy link
Member

jehiah commented Aug 7, 2015

For the counter page, i'm getting Zero's back from the /api/counter endpoint, but also the graphite URLS have unexpected &amp instead of just &.

Did you figure this out?

Yes, it was broken handling of message_count also affecting the Message Count columns (i just didn't notice on my dev instance that it was always Zero). Fixed by f9cdd4f

@mreiferson
Copy link
Member Author

cool 🔨

@jehiah
Copy link
Member

jehiah commented Aug 7, 2015

@mreiferson font issues addressed; take a look at last two commits?

@mreiferson
Copy link
Member Author

@jehiah don't you think the console.log might be useful when people inevitably encounter bugs?

@mreiferson
Copy link
Member Author

also, do we need another HTTP route for the same handler? Is it because bootstrap expects them at that URL?

@jehiah
Copy link
Member

jehiah commented Aug 7, 2015

yeah, bootstrap requests them as fonts/.... and that felt like the easiest path to resolving since the gulp step currently flattens directories. I'd rather one line of Go to more gulp =)

I have "no console.log in production" burned into my brain since it's not present in all browsers and thus causes JS errors (Thanks IE 9!)

@mreiferson
Copy link
Member Author

very well, squash time?

@jehiah jehiah force-pushed the nsqadmin_importable_323 branch from 3f9025b to d30fdc6 Compare August 7, 2015 15:48
jehiah added a commit that referenced this pull request Aug 7, 2015
@jehiah jehiah merged commit 925f82a into nsqio:master Aug 7, 2015
@mreiferson
Copy link
Member Author

😱

@klucar
Copy link

klucar commented Feb 5, 2016

Has anyone started work on refactoring the UI for #443? We get i/o timeout errors when nsq is under heavy load. I'd like to get rid of this timeout, make it configurable or ?

I have experience with doing ReactJS interfaces and this is kind of a perfect use-case for it. If nobody is already working this, I'll give it a go.

@mreiferson
Copy link
Member Author

@klucar we made a bunch of improvements recently in #701 that should significantly improve this situation, if you're interested in deploying an alpha

@klucar
Copy link

klucar commented Feb 5, 2016

we're just evaluating if NSQ can work/scale for us, so I'll gladly give an alpha a shot :)

@klucar
Copy link

klucar commented Feb 8, 2016

@mreiferson any idea when a new release will be cut?

@mreiferson
Copy link
Member Author

@klucar I've been lazy, we should get one out (I've been running it in production without issue for a few weeks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants