feat(asset): add asset data source with utilization query #52
Conversation
TigranVardanyan
requested review from
mure and
cameronwaterman
as code owners
|
Please fill out the other sections of the PR template and update your PR name to comply with the required format documented in the README. |
TigranVardanyan
changed the title
cameronwaterman
changed the title
There was a problem hiding this comment.
It feels a little odd to me to have a Asset Utilization data source. Following existing patterns, I think it would fit better as simply an Asset data source that can display utilization, etc. data via switching between items in a radio button group. Thoughts @mure ?
There was a problem hiding this comment.
Let's get some tests written for this file.
There was a problem hiding this comment.
Let's get some tests written for this file.
| DataSourceInstanceSettings, | ||
| dateTime, | ||
| } from '@grafana/data'; | ||
| import {BackendSrv, getBackendSrv, getTemplateSrv, TemplateSrv, TestingStatus} from '@grafana/runtime'; |
There was a problem hiding this comment.
| import {BackendSrv, getBackendSrv, getTemplateSrv, TemplateSrv, TestingStatus} from '@grafana/runtime'; | |
| import { BackendSrv, getBackendSrv, getTemplateSrv, TemplateSrv, TestingStatus } from '@grafana/runtime'; |
| dateTime, | ||
| } from '@grafana/data'; | ||
| import {BackendSrv, getBackendSrv, getTemplateSrv, TemplateSrv, TestingStatus} from '@grafana/runtime'; | ||
| import {DataSourceBase} from 'core/DataSourceBase'; |
There was a problem hiding this comment.
| import {DataSourceBase} from 'core/DataSourceBase'; | |
| import { DataSourceBase } from 'core/DataSourceBase'; |
| /** | ||
| * Return smallest start and biggest end of given array of utilization intervals | ||
| * @param data {Array<Interval<any>>} - Array of utilization intervals | ||
| * @returns {[Date, Date]} - | ||
| */ |
There was a problem hiding this comment.
Why does this function have documentation but none of the others do?
| return mergedIntervals; | ||
| } | ||
|
|
||
| export const getBusinessHours = ( |
There was a problem hiding this comment.
This is a super long function, could it be split up?
There was a problem hiding this comment.
changed to another super long function with some description
|
|
||
|
|
| * @returns {[Date, Date]} - | ||
| */ | ||
| export const getStartEndDates = (data: Array<Interval<any>>) => { | ||
|
|
| BOTH = 'BOTH' | ||
| } | ||
|
|
||
| export enum IsPeak { |
There was a problem hiding this comment.
This is an enum but reads like a boolean to me.
There was a problem hiding this comment.
let's leave everything as it is
maybe in the future we will add an option to show all without exception
|
It's probably not worth it now, but going forward it would be preferable that a change of this size gets split in to several PRs. |
Agreed, this is feedback I had also provided |
| return Boolean(query.assetIdentifier && query.minionId) | ||
| } | ||
|
|
||
| processQuery(query: AssetUtilizationQuery): ValidAssetUtilizationQuery { |
There was a problem hiding this comment.
This function isn't needed - the base class will take care of applying the defaults.
There was a problem hiding this comment.
We have a prettier configuration in the repo that should take care of this. There's not an NPM script for it, but you should be able to run npx prettier [file] --write. There's also a prettier extension for VSCode.
| const from = (new Date(range!.from.valueOf())).toISOString(); | ||
| const to = (new Date(range!.to.valueOf())).toISOString(); |
There was a problem hiding this comment.
Why are we creating a new Date object both here and when we pass it to onLoadUtilizationHistory? Can we simplify this?
Add asset metadata section Relocate certain functions to the core section for better modularity Fixed paddings Add missing return types of functions Correct the calculation algorithm Minor improvements
|
@TigranVardanyan I updated your branch from main to pull in an update of the plugin toolkit and fixed a type error |
|
Looks like one of the error tests is failing. Not sure if it's related to the merge from main? |
| import { DateTime, dateTime } from "@grafana/data"; | ||
|
|
||
| export interface AssetQuery extends DataQuery { | ||
| assetQueryType: AssetQueryType, |
There was a problem hiding this comment.
nit: Just call this property type since it's already under the AssetQuery interface
| assetQueryType: AssetQueryType, | |
| type: AssetQueryType, |
| export type ColumnDataType = 'BOOL' | 'INT32' | 'INT64' | 'FLOAT32' | 'FLOAT64' | 'STRING' | 'TIMESTAMP'; | ||
|
|
||
| export interface Column { | ||
| name: string; | ||
| dataType: ColumnDataType; | ||
| columnType: 'INDEX' | 'NULLABLE' | 'NORMAL'; | ||
| properties: Record<string, string>; | ||
| } | ||
|
|
||
| export type NumberTuple = [number, number] |
There was a problem hiding this comment.
These seem like types from the data frame plugin, can we delete? Also I don't see the NumberTuple type used anywhere.
| 'startTimestamp': T, | ||
| 'endTimestamp': T |
There was a problem hiding this comment.
| 'startTimestamp': T, | |
| 'endTimestamp': T | |
| startTimestamp: T, | |
| endTimestamp: T |
There was a problem hiding this comment.
I'm surprised the linter doesn't catch this. We don't use quotes in the property names of interfaces or object literals usually. There's a few places this applies to.
There was a problem hiding this comment.
done in all places
| export interface SystemLinkError { | ||
| error: { | ||
| args: string[]; | ||
| code: number; | ||
| message: string; | ||
| name: string; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is duplicated in the core utils, please delete
| export interface TableMetadata { | ||
| columns: Column[]; | ||
| id: string; | ||
| name: string; | ||
| workspace: string; | ||
| } |
There was a problem hiding this comment.
More data frame types that should be deleted
| if (query.workspace) { | ||
| filterString += `workspace = "${query.workspace}"`; | ||
| } | ||
| let systemMetadata = await this.post<{ data: SystemMetadata[] }>(this.sysmgmtUrl + '/query-systems', { |
There was a problem hiding this comment.
This should use the shared function for querying systems—add more params to the function if needed.
| } catch (error) { | ||
| throw new Error(`An error occurred while retrieving utilization history: ${error}`); | ||
| } | ||
| } while (continuationToken && (maxDataPoints && requestCount < maxDataPoints)); |
There was a problem hiding this comment.
options.maxDataPoints defaults to the width of the panel, which is usually 100s of pixels. Are we ever actually making that many requests here? That seems weird to me.
| 'endTime': momentToTime(dateTime(query.nonPeakStart)) | ||
| } | ||
| const utilizationFilter: string = utilizationCategory === UtilizationCategory.TEST ? `Category = \"Test\" or Category = \"test\"` : ''; | ||
| let arrayOfEntityIds: string[]; |
There was a problem hiding this comment.
| let arrayOfEntityIds: string[]; | |
| let entityIds: string[]; |
We don't usually put the type of a variable in its name
|
|
||
| const buildQuery = getQueryBuilder<AssetQuery>()({}); | ||
|
|
||
| mockTimers(); |
There was a problem hiding this comment.
This isn't necessary—you only need this if you're testing code that relies on window.setTimeout
|
|
||
| await expect(ds.query(buildQuery(assetUtilizationQueryMock))).rejects.toThrow() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
We should have tests that verify the output of the utilization queries. There's a lot of logic in the data source for building up filters and parsing responses that needs coverage. You can use Jest's snapshot testing to make it really easy to assert on the output. That's what we do in the tag data source:
I would also want to see a test that verifies that we replace template variables in queries correctly.
There was a problem hiding this comment.
Can you add an entry to provisioning/example.yaml for the assets plugin?
|
Closing this PR as it's being split up into separate changes |
Pull Request
🤨 Rationale
Asset data source with utilization query
👩💻 Implementation
Complexity - utilization calculations based on raw data( time zone offsets, overlapping asset usage, splitting data by day/hour )
Alternative solutions - instead of using the 'niapm/v1/query-asset-utilization-history' endpoint with additional calculations, the 'niapm/v1/query-asset-utilization' endpoint can be utilized(returns only peak utilization data).
Performance impact - despite the numerous calculations in the code, performance is not compromised, as there are relatively few server requests involved.
🧪 Testing
Manually reviewed and contrasted the output of the plugin with the data from the 'niapm/v1/query-asset-utilization' endpoint and the 'Advanced Asset Utilization [8h].ipynb' notebook.
✅ Checklist