Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Jul 27, 2022

It's time. Closes #1051

2022-08-30-metrics-finalish

Limitations/issues

  • Need to show full selected date range on x-axis, not just the range of the data we actually got back
  • Bug where validation error doesn't get cleared when you go from custom to preset
  • Only fetching 1 page of 1000 data points (won't fix in this PR, except maybe we could bump the page size)

@vercel
Copy link

vercel bot commented Jul 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
console-ui-storybook ✅ Ready (Inspect) Visit Preview Aug 31, 2022 at 5:24PM (UTC)

@david-crespo
Copy link
Collaborator Author

image

crying-cry

@david-crespo
Copy link
Collaborator Author

image

@david-crespo
Copy link
Collaborator Author

2022-08-26-graph-tooltip

@leftwo
Copy link

leftwo commented Aug 30, 2022

Can I run this change on folgers and see how it works with "real" data?

@david-crespo
Copy link
Collaborator Author

Yeah, definitely, though it will be easier to do after I merge this and update the pinned console in Omicron. Two undone todos you will definitely run into:

  • We are only fetching one page of 1000 data points. If there are more the chart will be incomplete. We can easily bump that up to the max page size of 10k, but to solve this in a general way we will need to reduce the number of data points server side so that they'll fit in a single page (see my comment on RFD 304 about this)
  • Rather than setting the x-axis of the graphs to show the entire selected range, it is set to auto, which computes the range from the available data points. So for example if you select a month but there's only data for a week, the graph should show the whole month with most of it empty but it won't.

vite.config.ts Outdated
// plugins:
// mode === 'development' ? ['./libs/babel-transform-react-display-name'] : [],
// },
// }),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ran into an issue where it was trying to put a displayName on my hook and I have no idea why. Will look into it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's trying to put a displayName

dateTimeRangePicker.displayName = "dateTimeRangePicker - app/components/form/fields/useDateTimeRangePicker.tsx";

on this line, which is an element, not a component

const dateTimeRangePicker = <Formik ... />

Got a minimal repro. Holy shit AST Explorer rules.

https://astexplorer.net/#/gist/d6da05485105d9b03cdf70559acc92fa/c7774e3ecda85343141fa21b1c4cc81555a73e36

Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be happening for top level components meaning one of the escape hatches is failing. I'm fiddling with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I resolved this in #1133. There needs to be another follow up to fix an invalid base assumption, but the proposed fix should be sufficient.

Comment on lines +82 to +83
{/* TODO: need a nicer way of saying what the boot disk is */}
Boot disk ( <code>{diskName}</code> )
Copy link
Contributor

Choose a reason for hiding this comment

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

Forever a challenge, ha.

const longDateTime = (ts: number) => format(new Date(ts), 'MMM d, yyyy HH:mm:ss zz')

// TODO: change these to theme colors so they work in light mode
const LIGHT_GRAY = 'var(--base-grey-600)'
Copy link
Contributor

Choose a reason for hiding this comment

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

The closest I can find that makes sense here is chart-fill-inactive which maps to base-black-600. Not sure if that's right or not though.

Copy link

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

the activated stat is really an internal stat for the upstairs, and I don't think we need to expose that to the end user.

Comment on lines +25 to +26
// TODO: we're only pulling the first page. Should we bump the cap to 10k?
// Fetch multiple pages if 10k is not enough? That's a bit much.
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be interesting to see the granularity of data required to get meaningful visualizations at different time ranges. I really hope 10k isn't necessary, that seems like an awfully large payload.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think if we were doing moving averages server-side to reduce the size of the result set, I would never choose to get that many points back.

Copy link
Contributor

@zephraph zephraph left a comment

Choose a reason for hiding this comment

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

Great start. I know there are still a lot of TODOs and follow up work, but I'm all for rolling forward.

@david-crespo david-crespo merged commit fd2935c into main Aug 31, 2022
@david-crespo david-crespo deleted the babbys-first-metric branch August 31, 2022 18:34
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.

Display one (1) disk metric

4 participants