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.

Sign up for GitHub

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

Jump to bottom

server: rewrite telemetry tests #46730

Closed
RaduBerinde opened this issue Mar 30, 2020 · 3 comments
Closed

server: rewrite telemetry tests #46730

RaduBerinde opened this issue Mar 30, 2020 · 3 comments
Assignees
@RaduBerinde
Labels
no-issue-activity

Comments

@RaduBerinde
Copy link
Member

The telemetry tests in pkg/server/updates_test.go are very hard to work with which limits the testing area. These should be rewritten using a datadriven approach. We can separate the mock recorder in a package and use it to output a list of counters that changed; the actual tests could then be moved to more appropriate packages (e.g. sqltelemetry).
@RaduBerinde RaduBerinde self-assigned this Mar 30, 2020
@otan
Copy link
Contributor

otan commented Mar 30, 2020

i was looking at doing something like kvtrace and SELECT the feature counts before/after a statement was executed from the crdb_internal.feature_counts table, and comparing that and giving the deltas, e.g. 
query TD telemetrytrace(sql.schema.something, sql.schema.something_else)
CREATE TABLE a (a int)
----
sql.schema.something 1
sql.schema.something_else 2
# note other telemetry increment targets are ignored

your solution of having an entry point into a mock recorder sounds more clean though

@RaduBerinde RaduBerinde mentioned this issue Mar 30, 2020
Merged
@asubiotto asubiotto mentioned this issue Mar 31, 2020
Merged
craig bot pushed a commit that referenced this issue Apr 3, 2020
@asubiotto
Merge #46778
976115c 
46778: sqltelemetry: add CANCEL QUERIES and CANCEL SESSIONS telemetry r=yuzefovich a=asubiotto

Release note: None (internal telemetry change)

Closes #45103

Not sure whether we want to distinguish between CANCEL QUERY vs QUERIES use and CANCEL SESSION vs SESSIONS use (or even how to). So just added one counter for each statement type which is probably good enough.

Tested this manually but holding off on writing tests since I've heard some work will be done on #46730 

Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
@RaduBerinde RaduBerinde mentioned this issue Apr 7, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Apr 7, 2020
@RaduBerinde
sql: add datadriven telemetry tests
0ca3976 
This commit adds `sql.TestTelemetry` which uses a datadriven approach to
telemetry testing. See the comment for that function for more details.

`TestCBOReportUsage` is converted to a datadriven test. Similar tests should be
moved over in subsequent changes.

Informs cockroachdb#46730.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Apr 8, 2020
@RaduBerinde
sql: add datadriven telemetry tests
73b40cc 
This commit adds `sql.TestTelemetry` which uses a datadriven approach to
telemetry testing. See the comment for that function for more details.

`TestCBOReportUsage` is converted to a datadriven test. Similar tests should be
moved over in subsequent changes.

Informs cockroachdb#46730.

Release note: None
craig bot pushed a commit that referenced this issue Apr 9, 2020
@RaduBerinde
Merge #47133
59d8c9d 
47133: sql: datadriven telemetry tests r=RaduBerinde a=RaduBerinde

#### server: clean up telemetry URL code

This change moves the code related to diagnostics URLs to `diagnosicspb` and
adds infrastructure to override the URLs via testing knobs (rather than fiddling
with global values).

Release note: None

#### server: move mockRecorder

Move `mockRecorder` to `diagutil.Server` and clean up the API.

Release note: None

#### sql: add datadriven telemetry tests

This commit adds `sql.TestTelemetry` which uses a datadriven approach to
telemetry testing. See the comment for that function for more details.

`TestCBOReportUsage` is converted to a datadriven test. Similar tests should be
moved over in subsequent changes.

Informs #46730.

Release note: None


Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Apr 17, 2020
@asubiotto
Merge #46778
64d4df4 
46778: sqltelemetry: add CANCEL QUERIES and CANCEL SESSIONS telemetry r=yuzefovich a=asubiotto

Release note: None (internal telemetry change)

Closes #45103

Not sure whether we want to distinguish between CANCEL QUERY vs QUERIES use and CANCEL SESSION vs SESSIONS use (or even how to). So just added one counter for each statement type which is probably good enough.

Tested this manually but holding off on writing tests since I've heard some work will be done on #46730 

Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
@asubiotto asubiotto mentioned this issue Apr 17, 2020
Merged
@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@RaduBerinde
Copy link
Member Author

We converted part of the tests, I don't think there are any plans for work here any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaduBerinde RaduBerinde

Labels
no-issue-activity
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@otan @RaduBerinde