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 a new optional secondary logger for internal slow queries #52377

Merged
merged 1 commit into from Aug 19, 2020

Conversation

arulajmani
Copy link
Collaborator

Previously, all slow queries were logged to the slow query log,
regardless of whether they were internal or from the user. This made
it easy for user queries to get lost in the log. This patch adds
an optional cluster setting, sql.log.slow_query.internal. When set to
true, all internal queries that have service latency above the slow
query log threshold will be logged to
cockroach-slow-log-internal-only.log instead of
cockroach-slow-log.log. This new setting is a no-op if the slow query
log hasn't been turned on, i.e there is no latency threshold set.

Closes #50201

Release note (sql change): adds a new cluster setting
sql.log.slow_query.internal which when turned on in conjunction with
the slow query log causes internal queries to be logged to an internal
query only slow query log instead of the main slow query log.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@awoods187
Copy link
Contributor

Is the new default behavior that internal are not shown? I think that should be the default and you should need to opt into showing internal.

@arulajmani
Copy link
Collaborator Author

Updated the implementation to completely remove internal queries from the slow query log. Now, only if both the latency threshold and the new internal slow query log opt in cluster setting is set will internal queries be logged to the new log.

Previously, all slow queries were logged to the slow query log,
regardless of whether they were internal or from the user. This made
it easy for user queries to get lost in the log.

This patch removes internal queries from the slow query log and adds a
new optional opt-in cluster setting,
`sql.log.slow_query.internal_queries.enabled`. When set to
true, all internal queries that have service latency above the slow
query log threshold will be logged to
`cockroach-sql-slow-internal-only.log`. This new setting is a no-op if
the slow query log hasn't been turned on, i.e there is
no latency threshold set.

Closes cockroachdb#50201

Release note (sql change): adds a new cluster setting
`sql.log.slow_query.internal` which when turned on in conjunction with
the slow query log causes internal queries to be logged to an internal
query only slow query log, at `cockroach-sql-slow-internal-only.log`.
Internal queries are no longer logged to the slow query log. This new
setting is opt-in, so default behavior is to not log internal slow
queries.
@arulajmani
Copy link
Collaborator Author

Rebased this as it had a conflict. Should be good for a review @solongordon

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)


pkg/sql/exec_log.go, line 211 at r1 (raw file):

		if execType == executorTypeInternal && slowInternalQueryLogEnabled {
			logger = p.execCfg.SlowInternalQueryLogger
		}

Hm, it seems like this is written in such a way that if the internal query log is not enabled, internal queries will still end up in the regular slow query log. Could you check that?

Copy link
Collaborator Author

@arulajmani arulajmani 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 @solongordon)


pkg/sql/exec_log.go, line 211 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Hm, it seems like this is written in such a way that if the internal query log is not enabled, internal queries will still end up in the regular slow query log. Could you check that?

I think logger would be nil in that case, as we won't enter any if condition, and therefore nothing will be logged, right? I tested it out to confirm, and it works as expected --

  • external queries are logged to the regular slow query log if they don't meet the threshold (and the threshold is set).
  • internal queries are logged to the internal slow query log if they don't meet the threshold (and the threshold is set) AND the user has explicitly opted into logging internal queries.

Copy link
Contributor

@solongordon solongordon 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


pkg/sql/exec_log.go, line 211 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I think logger would be nil in that case, as we won't enter any if condition, and therefore nothing will be logged, right? I tested it out to confirm, and it works as expected --

  • external queries are logged to the regular slow query log if they don't meet the threshold (and the threshold is set).
  • internal queries are logged to the internal slow query log if they don't meet the threshold (and the threshold is set) AND the user has explicitly opted into logging internal queries.

Oh you're totally right, I misread. Carry on. 🙂

@arulajmani
Copy link
Collaborator Author

TFTR!

bors r=solongordon

@craig
Copy link
Contributor

craig bot commented Aug 19, 2020

Build succeeded:

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.

sql: add option to remove internal queries from slow query log
4 participants