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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: Longview Memory Gauge #5615

Merged
merged 5 commits into from
Oct 29, 2019

Conversation

martinmckenna
Copy link
Contributor

@martinmckenna martinmckenna commented Oct 17, 2019

馃毃 This code got a little muddy. I think in follow up PR, I'm going to add logic to the Gauge to say

if (value === 0 && max === 0) { max = 1 }

so that we end up with a full grey bar if the stat is actually sitting at 0. See this commit in the next upcoming PR 馃毃

Description

Adds Longview Memory Gauge

Type of Change

  • Non breaking change ('update', 'change')

Note to Reviewers

I tried deleting my swap disk and seeing the dataset that is returned from the Longview API. It appears that if you don't have a certain disk, the response will still return the same keys but just have a value of 0.

I also initialized all values as undefined so that value || 1 logic isn't applied when a value is actually 0.

@martinmckenna martinmckenna self-assigned this Oct 17, 2019
/** convert KB to bytes */
usedMemoryToBytes,
{
unit: usedMemoryToBytes > howManyBytesInGB ? 'GB' : 'MB'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnwcallahan the maxUnit option wasn't working out correctly, so I went with this as a workaround. The problem I was seeing was that once the value reached 1GB, it was still returning MB.

Copy link
Contributor

Choose a reason for hiding this comment

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

maxUnit should cap the unit. So maxUnit: 'MB' should return the value up to MB, but never higher. Could you just not specify a unit here and use what it gives you back?

Here are the unit tests for that option. If there is a bug, could you write a failing test?

it("doesn't return units higher than the specific max unit", () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use both MB and GB, so I set the unit as GB and I was still getting MB for memory >= 1GB

Copy link
Contributor

Choose a reason for hiding this comment

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

The unit option lets you explicitly specify a unit for the result. So specifying{ unit: 'GB' } should always give you GB. Here's a test that confirms:

Was your input still in bytes (as it should always be)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I had { unit: 'MB', maxUnit: 'GB' }. I might have to retry this again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep looking back on this, it seems like you can't use both maxUnit and unit at the same time. I think the solution I have in place should be fine.

@@ -19,7 +19,7 @@
"start:docker": "lerna run build --stream --scope linode-js-sdk && yarn start:all",
"start:all": "lerna run start --parallel -- --color",
"clean": "rm -rf node_modules && lerna clean --yes",
"test": "lerna run test --stream --scope linode-manager -- --color",
"test": "lerna run test --stream --scope linode-manager -- --color --detectOpenHandles",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jestjs/jest#7287

This is supposed to show better error messaging when an async operation is not cleaned up.

/** convert KB to bytes */
usedMemoryToBytes,
{
unit: usedMemoryToBytes > howManyBytesInGB ? 'GB' : 'MB'
Copy link
Contributor

Choose a reason for hiding this comment

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

maxUnit should cap the unit. So maxUnit: 'MB' should return the value up to MB, but never higher. Could you just not specify a unit here and use what it gives you back?

Here are the unit tests for that option. If there is a bug, could you write a failing test?

it("doesn't return units higher than the specific max unit", () => {

Copy link
Contributor

@Jskobos Jskobos left a comment

Choose a reason for hiding this comment

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

I mentioned this in John's Storage PR, the same thing happens here too: if a gauge goes from Error state --> Empty state, the Error text in the innerText remains, while any non-empty states update appropriately.

@martinmckenna martinmckenna merged commit 05d8ed7 into linode:develop Oct 29, 2019
@johnwcallahan
Copy link
Contributor

yep looking back on this, it seems like you can't use both maxUnit and unit at the same time. I think the solution I have in place should be fine.

The solution you have is fine, but you shouldn't need to pass in both maxUnit and unit. By passing in { unit: 'MB' } you are explicitly stating you want the output in MB, and maxUnit doesn't do anything. This is a note to myself to clean up docs for this.

@martinmckenna martinmckenna deleted the M3-longview-memory branch October 30, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants