-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-20.2: server: add diagnostics support for tenants #58517
release-20.2: server: add diagnostics support for tenants #58517
Conversation
426fb84
to
b118aaa
Compare
Clean backport, right? An extra commit (roachtest) seems to have snuck in; if you wanted to backport that implicitly a) probably better in a separate PR (though I'm ok with it now that it's already here) and b) just mention in the commit msg that it's intentional. It looks like you caught an import cycle somewhere, so some manual fixup seems to be necessary. |
This commit contains preparatory changes for supporting tenants in our diagnostics reporting code. Tenants run one or more SQL-only instances rather than full CRDB nodes. This commit is intended to be backported to a 20.2 patch release, so it isolates changes that might affect other production code that's unrelated to tenants. That includes adding an optional field to the DiagnosticsReport proto and updating BuildReportingURL to use it. It also renames sqlServer to SQLServer, so that it can be returned from the exported StartTenant method. Release note: None
b118aaa
to
84a1001
Compare
Yes, I had to do a bunch of small changes due to various re-factorings that have happened in the 21.1 codebase. I didn't intend to add that 3rd commit - it seems that was |
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.
Also, there was one substantive change I made in PeriodicallyReportDiagnostics
. For this backport, I always run ReportDiagnostics
rather than testing the diagnostics.reporting.interval
cluster setting first. You can see the comment in that function explaining why.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @tbg)
Previously, SQL-only tenants did not report feature usage to the registration server. This commit adds that support via the creation of a new diagnostics package that encapsulates the usage report code in updates.go. However, since this commit needs to be backported to 20.2, the new code is *only* used with tenants, and is side-by-side with the existing code, which will continue to operate as before. A follow-on commit (that will not be backported) will fully replace the old implementation with the new. Release note (sql change): Multi-tenant clusters will now send anonymous usage information to the central CRDB registration server.
84a1001
to
dde7719
Compare
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.
Reviewed 9 of 9 files at r1, 19 of 19 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)
Backport 2/2 commits from #58399.
/cc @cockroachdb/release
Previously, SQL-only tenants did not report feature usage to the registration
server. This commit adds that support via the creation of a new diagnostics
package that encapsulates the usage report code in updates.go.
However, since this commit needs to be backported to 20.2, the new code is
only used with tenants, and is side-by-side with the existing code, which
will continue to operate as before. A follow-on commit (that will not be
backported) will fully replace the old implementation with the new.
Release note (sql change): Multi-tenant clusters will now send anonymous usage
information to the central CRDB registration server.