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

chore: revert messaging regarding query count sources #6941

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

david-rusnak
Copy link
Contributor

@btasker rolled out the fix that includes SQL queries in the query count usage vector (thanks Ben!), meaning we can get rid of the tooltip that explained that the query count only shows flux queries in the UI.

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

@david-rusnak david-rusnak requested review from a team as code owners September 3, 2024 21:46
Copy link
Contributor

@wdoconnell wdoconnell left a comment

Choose a reason for hiding this comment

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

Does Ben’s fix also cover InfluxQL queries?

@david-rusnak
Copy link
Contributor Author

@wdoconnell Good point 🤦 I'll ask tomorrow - completely forgot about that source

@btasker
Copy link

btasker commented Sep 4, 2024

@wdoconnell InfluxQL should already have been in there

  • The downsampling task checks requests against /query
  • Looks like the quartz query for the UI checks it too

(Ran a quick query against the downsampled data and the regions are reporting counts for that endpoint)

AFAIK, InfluxQL over Flight still goes to the DoGet endpoint, so those should be accounted for too.

@wdoconnell
Copy link
Contributor

@david-rusnak Looks good to me. This was just implemented with a git revert of #6910 correct?

Could you please force-push and run CI again? I think that was monitor-ci flake but want to confirm.

@wdoconnell wdoconnell force-pushed the revert-query-count-flux-only branch from 4359fe9 to 73f927d Compare September 9, 2024 14:32
@wdoconnell
Copy link
Contributor

Force pushed rebase against master that incorporates the removed subscriptionsUI test.

@wdoconnell wdoconnell force-pushed the revert-query-count-flux-only branch from 73f927d to b85a40c Compare September 9, 2024 14:57
@wdoconnell wdoconnell force-pushed the revert-query-count-flux-only branch from b85a40c to 6907740 Compare September 9, 2024 18:01
@wdoconnell wdoconnell added this pull request to the merge queue Sep 9, 2024
Merged via the queue into master with commit 66803f5 Sep 9, 2024
6 checks passed
@wdoconnell wdoconnell deleted the revert-query-count-flux-only branch September 9, 2024 19:38
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