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

fix: add guidance re usage query count #6910

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

wdoconnell
Copy link
Contributor

Clarifies that the query count on the C2 usage page reflects flux queries only.

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@wdoconnell wdoconnell requested a review from a team as a code owner July 22, 2024 16:29
@@ -39,6 +44,10 @@ const UsageSingleStat: FC<Props> = ({
decimalPlaces: {isEnforced: false, digits: 0},
}

// Adjusts table properties to warn user that only flux queries are included in the Query Count.
const isQueryCount: Boolean = usageVector.name === VectorName.QueryCount
const vectorName = isQueryCount ? 'Query Count (Flux Only)' : usageVector.name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This overrides the default backend-provided vector name.

eatondustin1
eatondustin1 previously approved these changes Jul 22, 2024
Copy link
Contributor

@eatondustin1 eatondustin1 left a comment

Choose a reason for hiding this comment

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

Looks good, but I did have one suggestion to mitigate future issues.

Comment on lines 44 to 49
export enum VectorName {
QueryCount = 'Query Count',
Storage = 'Storage',
DataIn = 'Data In',
DataOut = 'Data Out',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're only using 'Query Count' today, I think we're better off either inlining the 'Query Count' string in /src/usage/UsageSingleStat.tsx or exporting a const QueryCount = 'Query Count' here instead. Otherwise, we're maintaining the other vector names without any confirmation that they're correct, which could lead to someone mistakenly relying on their accuracy in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point -- this was really anticipating future use since I wasn't happy with the fact that we don't identify the actual string values anywhere in the UI. But there is no need for any of the enum values other than QueryCount now.

@wdoconnell
Copy link
Contributor Author

Amended commit addresses @eatondustin1 's comment above ^

@@ -2,7 +2,7 @@ import {Organization} from '../../../src/types'

const statHeaders = [
'Data In (MB)',
'Query Count',
'Query Count (Flux Only) ?',
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the ? here - doesn't the tooltip element cover that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a change I identified after running the ui integration tests, which failed without the change. This is part of a test that designates what text to look for in the header. QuestionMarkToolip is a Clockface component that, among other things, renders and formats a ? as text wherever the component is rendered. As a result, the test needed to be updated to address the fact that the text within the header now includes that ?.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense, thanks for the explanation!

@wdoconnell wdoconnell added this pull request to the merge queue Jul 22, 2024
Merged via the queue into master with commit 0f38e66 Jul 22, 2024
6 checks passed
@wdoconnell wdoconnell deleted the add_flux_query_note_to_usage_pg branch July 22, 2024 18:22
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.

3 participants