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: Stats trackers #4635

Merged
merged 9 commits into from
Sep 13, 2018
Merged

Conversation

DingoEatingFuzz
Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz commented Aug 31, 2018

This is part of a larger project to improve the cpu and memory usage charts in the UI

A StatsTracker is a new class that handles fetching data and using the resulting payload to append to an arbitrary number of long-lived stat arrays.

This may sound a lot like what Ember Data does, but this isn't using Ember Data. This data isn't stateful (no writes), and it isn't necessarily globally persistent (created and deleted on demand). Although fetching and normalizing the data does overlap in behavior with adapters and serializers, given the nature of client and allocation stats, I think it makes more sense to keep this data out of the Ember Data store.

The purpose of StatsTrackers is to build up data that can be used to power line charts for stats as stats are polled. StatsTrackers themselves don't create line charts nor do they control polling. The expectation is for a controller or component to use ember concurrency or something else to set up a polling loop that calls tracker.poll(), which handles fetching and appending of data. Then, the resulting data arrays in a tracker (tracker.get('cpu')) can be provided to a theoretical line chart component or d3.data call to build a chart with.

The structure of the StatsTracker classes are modeled after the Log suite of classes (Log, StreamLogger, and PollLogger). They are Ember Objects but exist outside of the registry/container environment. They expect dependencies (such as the token service) to be provided to them. Computed Property macros were written to make them easier to use.

The maxLength is enforced by removing elements from the head of the
list.
It follows the form of poll -> json parse -> append,
Where append is defined in subclasses to add data from the new frame
to long-lived rolling arrays of data.
It accumulates CPU and Memory usage for the allocation as a whole as well
as by task.
do {
yield this.get('stats').poll();
yield timeout(1000);
} while (!Ember.testing);
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

cpu: computed('allocation', function() {
return RollingArray(this.get('bufferSize'));
}),
memory: computed('allocation', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between these and the cpu + memory that are returned on the hash from the tasks computed prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are aggregates for the whole allocation. In the case where there is only one task, they're in theory the same value.

// the front of the array.

// Using Classes to extend Array is unsupported in Babel so this less
// ideal approach is taken: https://babeljs.io/docs/en/caveats#classes
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any additional implications since ember has prototype extensions on? Seems like you would still be able to call ArrayProxy methods on the returned array, is that right? And is it a concern? (tried to think of a way to prevent that but nothing's coming to mind)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are, which I have addressed in a future branch. Mostly addObject needs to behave like push does.

return returnValue;
};

array.unshift = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not a use case for it, but could be cool to support push and unshift (and probably not splice? or just push out both sides with splice?) and then you remove the surplus from the opposite side so that it's a SlideArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, that could be cool. I want to keep this as specific as possible though. The next thing I'm likely to address (probably in a different array type) is some sort of rolling aggregation. So there is still fine-grained second-by-second resolution for recent figures, but on the early end of the array, the resolution would be something like one point per 10 seconds, or one point per minute.

Still a nebulous brain noodle for now, but I may end up dabbling with it after I finish this first pass at everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, sorry bit late here, if you are avoiding closures, would Array.prototype.push.apply(this, arguments) help here? It would mean you wouldn't have to add your _push etc (polluting the array)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I hesitate to do that because there is no guarantee that array.push === Array.prototype.push, but in this case, the Array constructor is being invoked directly, so you're totally right that I can just use the methods on the prototype.

Good call, I'll make this change in my current branch 👍

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Nice - I think there's a couple of computed props you can remove, but all looks good otherwise. RollingArray is a nice primitive too - has me thinking about how we'll redo pagination in Vault... 🤔

@DingoEatingFuzz DingoEatingFuzz merged commit 1002eeb into f-ui-improved-stats-charts Sep 13, 2018
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui-stat-trackers branch September 13, 2018 23:41
@DingoEatingFuzz DingoEatingFuzz mentioned this pull request Sep 26, 2018
5 tasks
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants