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

sql: add telemetry for when distsql/vectorized is used #40885

Merged
merged 1 commit into from
Sep 19, 2019

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Sep 18, 2019

This adds two new features that are incremented whenever a query is
executed with distsql or vectorized. The feature names are:

  • sql.exec.query.is-distributed
  • sql.exec.query.is-vectorized

touches #40418 and cockroachlabs/registration#227

Release justification: low impact monitoring improvement

Release note: None

@rafiss rafiss requested review from rohany and a team September 18, 2019 20:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 18, 2019

I am not super sure about the feature names I choose -- let me know if another name would be better.

Copy link
Contributor

@rohany rohany left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rohany)


pkg/sql/sqltelemetry/exec.go, line 16 at r1 (raw file):

var (
	DistSQLExecCounter = telemetry.GetCounterOnce("sql.exec.distsql.on")

I'm not sure, but i think its better to place these in an init method? Though if there isn't any difference really then I don't have an opinion.

@rohany
Copy link
Contributor

rohany commented Sep 18, 2019

How about

sql.exec.query.is_distributed
sql.exec.query.is_vectorized

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks i like your names better

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany)


pkg/sql/sqltelemetry/exec.go, line 16 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I'm not sure, but i think its better to place these in an init method? Though if there isn't any difference really then I don't have an opinion.

my understanding was that an init method is only needed when the initialization can't be done inline, like if you need a loop. i'll check out other examples

@rohany
Copy link
Contributor

rohany commented Sep 18, 2019

jk make those names have dashes, not underscores -- telemetry/doc.go style guide

Copy link
Contributor

@rohany rohany left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


pkg/sql/sqltelemetry/exec.go, line 16 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

my understanding was that an init method is only needed when the initialization can't be done inline, like if you need a loop. i'll check out other examples

I looked a bit more, theres a small difference -- i think this initialization always happens, while the init initialization happens only if the package is imported

This adds two new features that are incremented whenever a query is
executed with distsql or vectorized. The feature names are:

- sql.exec.query.is-distributed
- sql.exec.query.is-vectorized

touches cockroachdb#40418

Release justification: low impact monitoring improvement

Release note: None
Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany)


pkg/sql/sqltelemetry/exec.go, line 16 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I looked a bit more, theres a small difference -- i think this initialization always happens, while the init initialization happens only if the package is imported

ah interesting. well the convention in the sqltelemetry package seems to be to do it inline with the declaration when possible, so i guess i'll stick with that.

e.g. pkg/sql/sqltelemetry/planning.go and pkg/sql/sqltelemetry/pgwire.go

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 18, 2019

ping @jordanlewis for TL sign-off

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 19, 2019

bors r=jordanlewis

craig bot pushed a commit that referenced this pull request Sep 19, 2019
40885: sql: add telemetry for when distsql/vectorized is used r=jordanlewis a=rafiss

This adds two new features that are incremented whenever a query is
executed with distsql or vectorized. The feature names are:

- sql.exec.query.is-distributed
- sql.exec.query.is-vectorized

touches #40418 and cockroachlabs/registration#227

Release justification: low impact monitoring improvement

Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Sep 19, 2019

Build succeeded

@craig craig bot merged commit c1bfd5c into cockroachdb:master Sep 19, 2019
@rafiss rafiss deleted the vec-telemetry branch September 27, 2019 19:19
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.

4 participants