-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add sql text counts stats to vtcombo
,vtgate
+vttablet
#15897
base: main
Are you sure you want to change the base?
Add sql text counts stats to vtcombo
,vtgate
+vttablet
#15897
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
52caf00
to
e8ba229
Compare
vtgate
+vttablet
vtcombo
,vtgate
+vttablet
Opened this one with a lot of failing CI to see if others have ideas how to debug The failures are confusing and feel unrelated to this change 🤔 |
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
There failures are related to the changes made. I have pushed a commit that fixes some of those. You should run the test locally and fix the remaining if any. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15897 +/- ##
==========================================
- Coverage 68.45% 68.44% -0.02%
==========================================
Files 1559 1559
Lines 196825 196837 +12
==========================================
- Hits 134736 134719 -17
- Misses 62089 62118 +29 ☔ View full report in Codecov by Sentry. |
@harshit-gangal indeed it is, thanks! 🤦 🙇
Good point, I've made a bad habit of relying on GitHub Actions CI because many Somehow this was not obvious from the CI outputs I looked at, including this + go-junit-report -set-exit-code
make: *** [Makefile:209: unit_test] Error 1
Error: Process completed with exit code 1. Perhaps it's just the v15 fork/release I'm working on, but I'm used to the |
The first step records the output, if you click on the step below in the CI you will see the output. |
@harshit-gangal ahh, that's where it goes! Thanks a lot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good change. Approving with the caveat that the question about the metric name should be addressed. We'll need one more review from @vitessio/query-serving
@@ -209,6 +209,11 @@ var ( | |||
"VtgateApiRowsAffected", | |||
"Rows affected by a write (DML) operation through the VTgate API", | |||
[]string{"Operation", "Keyspace", "DbType"}) | |||
|
|||
sqlTextCounts = stats.NewCountersWithMultiLabels( | |||
"VtgateSQLTextCounts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does /metrics
render this one? If it turns out to be vtgate_s_q_l_text_counts, we'll want to rename it :)
In general, for any metrics changes, it is good to post screenshots or embedded text from both /debug/vars
and /metrics
into the PR description. That serves as a visual check for what we are emitting.
I removed the website docs label. We don't actually have a page that lists all available metrics, so there's no good place to add docs for new metrics. |
Since the PR has two approvals I am adding the |
* Add sql text counts stats to `vtcombo`,`vtgate`+`vttablet` Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * initialize sqlTextCounts where missed Signed-off-by: Harshit Gangal <harshit@planetscale.com> * missing fix Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * fix accidental line delete Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Harshit Gangal <harshit@planetscale.com> Co-authored-by: Harshit Gangal <harshit@planetscale.com>
Description
This PR adds stats for the size of query text processed by
vtgate
andvttablet
This is useful to understand large queries that potentially cause performance, grpc message size, bandwidth problems
Related Issue(s)
Resolves #15929
Checklist
Deployment Notes