nsqadmin: display errors #421

Merged
merged 12 commits into from Sep 17, 2015

Projects

None yet

3 participants

@jehiah
Member
jehiah commented Jul 25, 2014

nsqadmin doesn't display when it's unable to query all the appropriate hosts to return stats. It should list them as errors at the top of the page.

(it can be missleading when the query to one host timesout but you don't realize it)

@jehiah jehiah added the bug label Jul 25, 2014
@mreiferson mreiferson added the 1.0 label Jul 27, 2014
@mreiferson mreiferson added contributor-friendly and removed 1.0 labels Aug 21, 2014
@mreiferson mreiferson added the 1.0 label Aug 10, 2015
@mreiferson
Member

work in progress:

screen shot 2015-08-29 at 2 57 54 pm

@jehiah
Member
jehiah commented Aug 31, 2015

@mreiferson this approach looks good

@mreiferson
Member

@rolyatmax would you mind taking a look at the JS error handling approaches here? ❤️

@mreiferson
Member

@jehiah want to give this a spin?

@rolyatmax rolyatmax commented on the diff Sep 11, 2015
nsqadmin/static/js/collections/nodes.js
@@ -2,7 +2,7 @@ var Backbone = require('backbone');
var AppState = require('../app_state');
-var Node = require('../models/node');
+var Node = require('../models/node'); //eslint-disable-line no-undef
@rolyatmax
rolyatmax Sep 11, 2015

You should add:

  "globals": {
    "module": true,
    "require": true
  }

to your .eslintrc.

@mreiferson
mreiferson Sep 11, 2015 Member

is it because Node is a restricted keyword?

@rolyatmax
rolyatmax Sep 11, 2015

Not a restricted keyword.

Nodejs is a holy word - maybe that's what you're thinking of?

@rolyatmax rolyatmax commented on the diff Sep 11, 2015
nsqadmin/static/js/models/node.js
var AppState = require('../app_state');
var Backbone = require('backbone');
-var Node = Backbone.Model.extend({
+var Node = Backbone.Model.extend({ //eslint-disable-line no-undef
@rolyatmax
rolyatmax Sep 11, 2015

Why do you need this? //eslint-disable-line no-undef

@rolyatmax rolyatmax and 1 other commented on an outdated diff Sep 11, 2015
nsqadmin/static/js/views/app.js
@@ -45,11 +46,11 @@ var AppView = BaseView.extend({
this.listenTo(Pubsub, 'view:ready', function() {
$('.rate').each(function() {
$.get(AppState.url('/graphite'), {
- 'metric': 'rate',
- 'target': $(this).attr('target')
- }).done(function(data) {
- $(this).html(data['rate']);
- }.bind(this));
+ 'metric': 'rate',
+ 'target': $(this).attr('target')
+ })
+ .done(function(data) { $(this).html(data['rate']); }.bind(this))
+ .fail(function() { $(this).html('ERROR'); }.bind(this));
});
@rolyatmax
rolyatmax Sep 11, 2015

If instead of relying on jQuery's special handling of this (which can get confusing - especially when nested within several functions), you can be a little more explicit (and lose the binds in the process) if you do something like this:

$('.rate').each(function(i, el) {
    $.get(AppState.url('/graphite'), {
        'metric': 'rate',
        'target': $(el).attr('target')
    })
    .done(function(data) { $(el).html(data['rate']); })
    .fail(function() { $(el).html('ERROR'); });
});

or even:

$('.rate').each(function(i, el) {
    var $el = $(el);
    $.get(AppState.url('/graphite'), {
        'metric': 'rate',
        'target': $el.attr('target')
    })
    .done(function(data) { $el.html(data['rate']); })
    .fail($el.html.bind($el, 'ERROR'));
});
@rolyatmax rolyatmax and 1 other commented on an outdated diff Sep 11, 2015
nsqadmin/static/js/views/app.js
@@ -133,9 +134,9 @@ var AppView = BaseView.extend({
var node = new Node({
'name': nodeName
});
- node.tombstoneTopic(topicName).done(function() {
- window.location.reload(true);
- });
+ node.tombstoneTopic(topicName)
+ .done(function() { window.location.reload(true); })
+ .fail(this.handleAJAXError.bind(this));
});
@rolyatmax
rolyatmax Sep 11, 2015

Does this actually work?

The this in .fail(this.handleAJAXError.bind(this)) is referring to whatever context bootbox uses when it calls this callback. (I'm guessing this will be incorrectly set to window - the result of calling an instance's method without a context.) You probably need to add .bind(this) to the bootbox callback, as well.

For example:

bootbox.confirm(txt, function(result) {
    ...
}.bind(this));
@rolyatmax rolyatmax and 1 other commented on an outdated diff Sep 11, 2015
nsqadmin/static/js/views/channel.js
- this.template = require('./channel.hbs');
- this.render();
- Pubsub.trigger('view:ready');
- }.bind(this));
+ this.model.fetch()
+ .done(function(data) {
+ this.template = require('./channel.hbs');
+ this.render({'message': data['message']});
+ }.bind(this))
+ .fail(function(jqXHR) {
+ this.template = require('./error.hbs');
+ this.handleViewError(jqXHR);
+ }.bind(this))
+ .always(function() {
+ Pubsub.trigger('view:ready');
+ });
@rolyatmax
rolyatmax Sep 11, 2015

Muchisimos tabs, Batman. Personally, I'd unindent the bodies of these functions by one tab. But that's just a style comment. Take it or leave it 😬

@rolyatmax
rolyatmax Sep 11, 2015

Also, should this.template = require('./error.hbs'); just go in the handleViewError method?

UPDATED:

See comment in handleViewError method definition.

@mreiferson
mreiferson Sep 11, 2015 Member

I couldn't find a style I liked, I'll play around with it

@rolyatmax
rolyatmax Sep 11, 2015

If you handled the templates like I was suggesting, you could do something like this:

this.model.fetch()
    .done(function(data) {
        this.render({'message': data['message']});
    }.bind(this))
    .fail(this.handleViewError.bind(this))
    .always(Pubsub.trigger.bind(Pubsub, 'view:ready'));

But you'd need to make sure template: require('./channel.hbs') was in your View definition.

If you didn't mind data's other properties getting into the render method, you could simplify further:

this.model.fetch()
    .done(this.render.bind(this))
    .fail(this.handleViewError.bind(this))
    .always(Pubsub.trigger.bind(Pubsub, 'view:ready'));

...though it's not super explicit that render and handleViewError are both receiving arguments as the done and fail callbacks.

@rolyatmax rolyatmax and 1 other commented on an outdated diff Sep 11, 2015
nsqadmin/static/js/views/base.js
+ msg = parsed['message'];
+ } catch(err) {
+ msg = 'ERROR: failed to decode JSON - ' + err.message;
+ }
+ }
+ return msg;
+ },
+
+ handleAJAXError: function(jqXHR) {
+ $('#warning, #error').hide();
+ $('#error .alert').text(this.parseErrorMessage(jqXHR));
+ $('#error').show();
+ },
+
+ handleViewError: function(jqXHR) {
+ this.render({'message': this.parseErrorMessage(jqXHR)});
@rolyatmax
rolyatmax Sep 11, 2015

I'm seeing something like this.template = require('./error.hbs') appear before each time this method is called. Mutating template on these views puts them in a weird state. It might be a better practice to make handleViewError something like:

var errorTemplate = require('./error.hbs');

...

handleViewError: function(jqXHR) {
    this.removeSubviews();
    this.$el.html(errorTemplate({'message': this.parseErrorMessage(jqXHR)}));
}

This also allows the error message to show up in the case that the view has renderOnce: true.

@mreiferson
mreiferson Sep 11, 2015 Member

I like this

@rolyatmax rolyatmax and 1 other commented on an outdated diff Sep 11, 2015
nsqadmin/static/js/views/lookup.js
@@ -20,14 +21,23 @@ var LookupView = BaseView.extend({
initialize: function() {
BaseView.prototype.initialize.apply(this, arguments);
- $.ajax(AppState.url('/topics?inactive=true')).done(function(data) {
- this.template = require('./lookup.hbs');
- this.render({
- 'topics': _.map(data['topics'], function(v, k) {
- return {'name': k, 'channels': v};
- })
- });
- }.bind(this));
+ $.ajax(AppState.url('/topics?inactive=true'))
+ .done(function(data) {
+ this.template = require('./lookup.hbs');
@rolyatmax
rolyatmax Sep 11, 2015

Swapping out templates like this seems a little strange to me (and prone to bugs). I usually have one template for a View. And if I need to insert spinners or error messages, I do that "manually". Typically, this ajax code would live in a model which would fire Pubsub events, like loading, and error. Views would then listen to these events and throw in error messages or spinners as needed.

All of this might seem to be more work than you want to do right now, however - especially if you're wanting to a rewrite (sayyyy... in React?)

@mreiferson
mreiferson Sep 11, 2015 Member

Let's talk about this separately from error handling, I'm open to (simple) improvements before another React-factoring.

@mreiferson
Member

updated @rolyatmax

@rolyatmax

Updates look good to me. Have you tried producing all these error states? 😬

@mreiferson
Member

most of them 👊

@rolyatmax

Sweeeeet - I'll leave the Go review for a GoPro.

@mreiferson
Member

@jehiah tested this a bunch against a real cluster - used https://github.com/apenwarr/sshuttle/ which is pretty amazing

@mreiferson
Member

OK, I think this is RFR @jehiah

@jehiah jehiah commented on an outdated diff Sep 17, 2015
nsqadmin/static/html/index.html
@@ -13,7 +14,7 @@
<div id="container"></div>
<script type="text/javascript">
- var USER_AGENT = 'nsqadmin';
+ var USER_AGENT = 'nsqadmin v{{.Version}}';
@jehiah
jehiah Sep 17, 2015 Member

nsqadmin/{{.Version}} to follow rfc7231 section 5.5.3

@jehiah
Member
jehiah commented Sep 17, 2015

LGTM aside from my nitpick. (sorry for the delay in review; i was offline for a few days)

@mreiferson
Member

no worries, fixed and squashed in (remembered to update bin dater too this time)

RFM

@jehiah jehiah merged commit 885b06f into nsqio:master Sep 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mreiferson mreiferson deleted the mreiferson:nsqadmin_errors_421 branch Sep 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment