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

UI: Client and Server Monitor #8177

Merged
merged 21 commits into from
Jun 18, 2020
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2eceffa
Create new monitor route for clients
DingoEatingFuzz Jun 13, 2020
cc8aed8
Monitor component and query param interaction
DingoEatingFuzz Jun 13, 2020
9e59488
Implement the log streaming portion of the AgentMonitor component
DingoEatingFuzz Jun 13, 2020
f02beee
Reimplement synchronizeScrollPosition in a way that actually works
DingoEatingFuzz Jun 13, 2020
0a4cde3
Refactor AgentMonitor levels handling to be simpler
DingoEatingFuzz Jun 13, 2020
47969c4
Inline-block the buttons to remove weird text-flow spacing
DingoEatingFuzz Jun 13, 2020
88e7e7e
Preserve the log when switching log levels on monitor
DingoEatingFuzz Jun 13, 2020
2c85e69
Temporary helpers for ember-power-select
DingoEatingFuzz Jun 15, 2020
f896a62
Test coverage for the AgentMonitor component
DingoEatingFuzz Jun 15, 2020
d0a0ffd
Test coverage for the client monitor page
DingoEatingFuzz Jun 16, 2020
a4a8c9e
Ability for agent:read
DingoEatingFuzz Jun 16, 2020
c3b4999
New component version of the forbidden-message partial
DingoEatingFuzz Jun 16, 2020
a0fce03
Show a helpful forbidden message when monitor access is not authorized
DingoEatingFuzz Jun 16, 2020
9e5cd53
Refactor the servers/server pages to match the subnav style of nested…
DingoEatingFuzz Jun 16, 2020
9294a7a
Add nested monitor route to servers/server
DingoEatingFuzz Jun 16, 2020
7185757
Redesign the server detail page to be inline with everything else
DingoEatingFuzz Jun 16, 2020
53027ba
Test coverage for new features of the server detail page
DingoEatingFuzz Jun 16, 2020
a0c6cc2
Server monitor page
DingoEatingFuzz Jun 16, 2020
5bb3c43
Select all shortcut support for the streaming file component
DingoEatingFuzz Jun 16, 2020
619aea8
Guard the request animation frame with the existing requestFrame flag
DingoEatingFuzz Jun 17, 2020
34e8e1f
Code review feedback
DingoEatingFuzz Jun 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions ui/app/abilities/agent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import AbstractAbility from './abstract';
import { computed, get } from '@ember/object';
import { or } from '@ember/object/computed';

export default class Client extends AbstractAbility {
@or('bypassAuthorization', 'selfTokenIsManagement', 'policiesIncludeAgentReadOrWrite')
canRead;

@computed('token.selfTokenPolicies.[]')
get policiesIncludeAgentReadOrWrite() {
const policies = (this.get('token.selfTokenPolicies') || [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also use get instead of this.get? I’m semi-surprised it doesn’t warn about this but maybe that’s because AbstractAbility is already @classic. (ETA I have since found it’s because I didn’t include the ESLint rules 😞)

.toArray()
.map(policy => get(policy, 'rulesJSON.Agent.Policy'))
.compact();

return policies.some(policy => policy === 'read' || policy === 'write');
}
}
71 changes: 71 additions & 0 deletions ui/app/components/agent-monitor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { computed } from '@ember/object';
import { assert } from '@ember/debug';
import { tagName } from '@ember-decorators/component';
import Log from 'nomad-ui/utils/classes/log';

const LEVELS = ['error', 'warn', 'info', 'debug', 'trace'];

@tagName('')
export default class AgentMonitor extends Component {
@service token;

client = null;
server = null;
level = LEVELS[2];
onLevelChange() {}

levels = LEVELS;
monitorUrl = '/v1/agent/monitor';
isStreaming = true;
logger = null;

@computed('level', 'client.id', 'server.id')
get monitorParams() {
assert(
'Provide a client OR a server to AgentMonitor, not both.',
this.server != null || this.client != null
);

const type = this.server ? 'server_id' : 'client_id';
const id = this.server ? this.server.id : this.client && this.client.id;

return {
log_level: this.level,
[type]: id,
};
}

didInsertElement() {
this.updateLogger();
}

updateLogger() {
let currentTail = this.logger ? this.logger.tail : '';
if (currentTail) {
currentTail += `\n...changing log level to ${this.level}...\n\n`;
}
this.set(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that using this.set means this needs to be @classic. I see now that I failed to follow the part of the instructions that would cause this to be a linting error, so I’m sorry about that, I’ll open a PR 😳

'logger',
Log.create({
logFetch: url => this.token.authorizedRequest(url),
params: this.monitorParams,
url: this.monitorUrl,
tail: currentTail,
})
);
}

setLevel(level) {
this.logger.stop();
this.set('level', level);
this.onLevelChange(level);
this.updateLogger();
}

toggleStream() {
this.set('streamMode', 'streaming');
this.toggleProperty('isStreaming');
}
}
5 changes: 5 additions & 0 deletions ui/app/components/client-subnav.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Component from '@ember/component';
import { tagName } from '@ember-decorators/component';

@tagName('')
export default class ClientSubnav extends Component {}
5 changes: 5 additions & 0 deletions ui/app/components/forbidden-message.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Component from '@ember/component';
import { tagName } from '@ember-decorators/component';

@tagName('')
export default class ForbiddenMessage extends Component {}
5 changes: 5 additions & 0 deletions ui/app/components/server-subnav.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Component from '@ember/component';
import { tagName } from '@ember-decorators/component';

@tagName('')
export default class ServerSubnav extends Component {}
57 changes: 46 additions & 11 deletions ui/app/components/streaming-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import WindowResizable from 'nomad-ui/mixins/window-resizable';
import { classNames, tagName } from '@ember-decorators/component';
import classic from 'ember-classic-decorator';

const A_KEY = 65;

@classic
@tagName('pre')
@classNames('cli-window')
Expand All @@ -14,6 +16,10 @@ export default class StreamingFile extends Component.extend(WindowResizable) {
mode = 'streaming'; // head, tail, streaming
isStreaming = true;
logger = null;
follow = true;

// Internal bookkeeping to avoid multiple scroll events on one frame
requestFrame = true;

didReceiveAttrs() {
if (!this.logger) {
Expand All @@ -26,12 +32,15 @@ export default class StreamingFile extends Component.extend(WindowResizable) {
performTask() {
switch (this.mode) {
case 'head':
this.set('follow', false);
this.head.perform();
break;
case 'tail':
this.set('follow', true);
this.tail.perform();
break;
case 'streaming':
this.set('follow', true);
if (this.isStreaming) {
this.stream.perform();
} else {
Expand All @@ -41,8 +50,42 @@ export default class StreamingFile extends Component.extend(WindowResizable) {
}
}

scrollHandler() {
const cli = this.element;
window.requestAnimationFrame(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be wrapped in an if to check on the value of requestFrame? It looks like the value of that field is updated but never checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haaaaaaaa yep. Thank you for catching this.

// If the scroll position is close enough to the bottom, autoscroll to the bottom
this.set('follow', cli.scrollHeight - cli.scrollTop - cli.clientHeight < 20);
this.requestFrame = true;
});
this.requestFrame = false;
}

keyDownHandler(e) {
// Rebind select-all shortcut to only select the text in the
// streaming file output.
if ((e.metaKey || e.ctrlKey) && e.keyCode === A_KEY) {
e.preventDefault();
const selection = window.getSelection();
selection.removeAllRanges();
const range = document.createRange();
range.selectNode(this.element);
selection.addRange(range);
}
}

didInsertElement() {
this.fillAvailableHeight();

this.set('_scrollHandler', this.scrollHandler.bind(this));
this.element.addEventListener('scroll', this._scrollHandler);

this.set('_keyDownHandler', this.keyDownHandler.bind(this));
document.addEventListener('keydown', this._keyDownHandler);
}

willDestroyElement() {
this.element.removeEventListener('scroll', this._scrollHandler);
document.removeEventListener('keydown', this._keyDownHandler);
}

windowResizeHandler() {
Expand All @@ -69,24 +112,16 @@ export default class StreamingFile extends Component.extend(WindowResizable) {

@task(function*() {
yield this.get('logger.gotoTail').perform();
run.scheduleOnce('afterRender', this, this.synchronizeScrollPosition, [true]);
})
tail;

synchronizeScrollPosition(force = false) {
const cliWindow = this.element;
if (cliWindow.scrollHeight - cliWindow.scrollTop < 10 || force) {
// If the window is approximately scrolled to the bottom, follow the log
cliWindow.scrollTop = cliWindow.scrollHeight;
synchronizeScrollPosition() {
if (this.follow) {
this.element.scrollTop = this.element.scrollHeight;
}
}

@task(function*() {
// Force the scroll position to the bottom of the window when starting streaming
this.logger.one('tick', () => {
run.scheduleOnce('afterRender', this, this.synchronizeScrollPosition, [true]);
});

// Follow the log if the scroll position is near the bottom of the cli window
this.logger.on('tick', this, 'scheduleScrollSynchronization');

Expand Down
9 changes: 9 additions & 0 deletions ui/app/controllers/clients/client/monitor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Controller from '@ember/controller';
import classic from 'ember-classic-decorator';

@classic
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be de-@classic-ed 🤓

export default class ClientMonitorController extends Controller {
queryParams = [{ level: 'level' }];

level = 'info';
}
28 changes: 1 addition & 27 deletions ui/app/controllers/servers.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,5 @@
import { alias } from '@ember/object/computed';
import Controller from '@ember/controller';
import Sortable from 'nomad-ui/mixins/sortable';

export default class ServersController extends Controller.extend(Sortable) {
@alias('model.nodes') nodes;
@alias('model.agents') agents;

queryParams = [
{
currentPage: 'page',
},
{
sortProperty: 'sort',
},
{
sortDescending: 'desc',
},
];

currentPage = 1;
pageSize = 8;

sortProperty = 'isLeader';
sortDescending = true;

export default class ServersController extends Controller {
isForbidden = false;

@alias('agents') listToSort;
@alias('listSorted') sortedAgents;
}
27 changes: 26 additions & 1 deletion ui/app/controllers/servers/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,32 @@
import { alias } from '@ember/object/computed';
import Controller, { inject as controller } from '@ember/controller';
import Sortable from 'nomad-ui/mixins/sortable';

export default class IndexController extends Controller {
export default class IndexController extends Controller.extend(Sortable) {
@controller('servers') serversController;
@alias('serversController.isForbidden') isForbidden;

@alias('model.nodes') nodes;
@alias('model.agents') agents;

queryParams = [
{
currentPage: 'page',
},
{
sortProperty: 'sort',
},
{
sortDescending: 'desc',
},
];

currentPage = 1;
pageSize = 8;

sortProperty = 'isLeader';
sortDescending = true;

@alias('agents') listToSort;
@alias('listSorted') sortedAgents;
}
9 changes: 9 additions & 0 deletions ui/app/controllers/servers/server/monitor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Controller from '@ember/controller';
import classic from 'ember-classic-decorator';

@classic
export default class ServerMonitorController extends Controller {
queryParams = [{ level: 'level' }];
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn’t really matter but I think this could be ['level'], the object format I think is only needed when relabeling a query parameter? Also maybe this doesn’t need to be @classic.


level = 'info';
}
8 changes: 6 additions & 2 deletions ui/app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,15 @@ Router.map(function() {
});

this.route('clients', function() {
this.route('client', { path: '/:node_id' });
this.route('client', { path: '/:node_id' }, function() {
this.route('monitor');
});
});

this.route('servers', function() {
this.route('server', { path: '/:agent_id' });
this.route('server', { path: '/:agent_id' }, function() {
this.route('monitor');
});
});

this.route('csi', function() {
Expand Down
35 changes: 1 addition & 34 deletions ui/app/routes/clients/client.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';
import { collect } from '@ember/object/computed';
import notifyError from 'nomad-ui/utils/notify-error';
import { watchRecord, watchRelationship } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default class ClientRoute extends Route.extend(WithWatchers) {
export default class ClientRoute extends Route {
@service store;

model() {
Expand All @@ -28,34 +25,4 @@ export default class ClientRoute extends Route.extend(WithWatchers) {
}
return model && model.get('allocations');
}

setupController(controller, model) {
controller.set('flagAsDraining', model && model.isDraining);

return super.setupController(...arguments);
}

resetController(controller) {
controller.setProperties({
eligibilityError: null,
stopDrainError: null,
drainError: null,
flagAsDraining: false,
showDrainNotification: false,
showDrainUpdateNotification: false,
showDrainStoppedNotification: false,
});
}

startWatchers(controller, model) {
if (model) {
controller.set('watchModel', this.watch.perform(model));
controller.set('watchAllocations', this.watchAllocations.perform(model));
}
}

@watchRecord('node') watch;
@watchRelationship('allocations') watchAllocations;

@collect('watch', 'watchAllocations') watchers;
}
Loading