-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add cloud instance viewer #1473
Add cloud instance viewer #1473
Conversation
… for use with instance viewer
} elseif (in_array($srcRecord['event_type_id'], $this->_start_event_ids)) { | ||
$this->updateInstance($srcRecord); | ||
} elseif (in_array($srcRecord['event_type_id'], $this->_stop_event_ids)) { | ||
} elseif (in_array($srcRecord['event_type_id'], [1,16])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded 1,16 seems out of place here since you have gone to the trouble of making the event ids php constants and then putting them in class member variables.
self::SHELVE, | ||
self::UNSHELVE, | ||
self::POWER_OFF_START, | ||
self::POWER_OFF, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is going to need some explanatory comments. How is POWER_OFF a start event? How come the start_event_ids and end_event_ids are exactly the same except for TERMINATE is not a start event?
$this->_end_time = $etlConfig->getVariableStore()->endDate ? date('Y-m-d H:i:s', strtotime($etlConfig->getVariableStore()->endDate)) : null; | ||
|
||
$this->resetInstance(); | ||
} | ||
|
||
private function initInstance($srcRecord) | ||
{ | ||
$default_end_time = isset($this->_end_time) ? $this->_end_time : $srcRecord['event_time_ts']; | ||
$beginOfDay = strtotime("today", $srcRecord['event_time_ts']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 'timezone' safe?
[ | ||
{ | ||
{ | ||
"0": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that recently merged PR #1481 re-indexes the arrays returned from getRealms() so that explicit indices are not needed here (previously there were potentially gaps in the array keys returned). This also applies to the realms.json testing artifact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, here's my first go through. Ultimately it looks good, most of things I found were with an eye to make our future selves hate our past selves a little less. The only thing of any substance that I found / thought of is the whole having to ultimately support multiple Cloud infrastructures thing that I think is more of a point of discussion and definitely not a show stopper.
const UNPAUSE_START = 56; | ||
const POWER_ON_START = 58; | ||
const UNSUSPEND_START = 60; | ||
const UNSHELVE_END = 63; | ||
const START = 2; | ||
const RESUME = 8; | ||
const STATE_REPORT = 16; | ||
const UNSHELVE = 20; | ||
const UNPAUSE = 57; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this event type used? I noticed that this variable wasn't referenced by anything.
} | ||
], | ||
"where": [ | ||
"sr.start_day_id <= ${:PERIOD_END_DAY_ID} AND sr.end_day_id >= ${:PERIOD_START_DAY_ID}", | ||
"sr.instance_type != \"Unknown\"", | ||
"sr.instance_id !=\"1\"", | ||
"sr.num_cores != 0" | ||
"sr.num_cores != 0", | ||
"sr.start_event_type_id IN (2,8,16,20,57,59,61)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get a comment w/ an explanation of where these numbers are pulled from, or where their definitions can be found? I'm guessing it's the CloudStateReconstructorTransformIngestor? But I'd imagine that'll make it easier for somebody to know how to go further down the rabbit hole in the future.
], | ||
"where": [ | ||
"sr.start_day_id <= ${:PERIOD_END_DAY_ID} AND sr.end_day_id >= ${:PERIOD_START_DAY_ID}", | ||
"sr.instance_type != \"Unknown\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a big deal, but it might be more readable if these were single quotes so as not to have to deal with escaping. Same for the line below this one.
@@ -14,7 +14,7 @@ | |||
"source_query": { | |||
"records": { | |||
"resource_id": "staging.resource_id", | |||
"asset_type_id": "staging.root_volume_type_id", | |||
"asset_type_id": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same vein as one of my comments above, a quick comment about what asset_type_id: 2
is in a human readable format would go a long way towards future readability.
@@ -11,6 +11,7 @@ | |||
"records": { | |||
"resource_id": "raw.resource_id", | |||
"account_id": "act.account_id", | |||
"asset_type_id": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- See above comment about future readability.
}, | ||
load_record: function (panel, record) { | ||
var self = this; | ||
var active_states = [2, 8, 16, 20, 57, 59, 61]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm wondering if it might be worth it to have a this data reside within a config file so that the data could be shared between the front & back end without having to manually make code changes? This obviously works but just a thought to maybe make any future changes easier. Another thing to keep in mind is that we're going to end up having to support multiple cloud's and each one is going to have different states etc. Maybe that's not something we worry about now, but it's something that we will have to address.
This PR adds functionality to track both active and inactive sessions and adds to the Job Viewer the ability to view details about a particular instance including all of the sessions of a VM on a timeline showing when they are active and inactive. Data for the Cloud realm should be re-ingested since we are tracking some new events.
The tests have changed to include the cloud realm in any test that checks to see what raw data realms exist. The regression tests have changed because stop events for an active session are no longer the
.end
event such ascompute.instance.power_off.end
but are now the.start
events likecompute.instance.power_off.start
. The time difference between these events in the test data is a few seconds slightly changing the core hours and wall hours.Motivation and Context
It is useful to see the lifecycle of a VM and also let's us track data about inactive sessions in the future.
Tests performed
Tested locally and on metrics-dev.
Checklist: