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

metrics: add different labels for restricted SQL and general SQL #7631

Merged
merged 4 commits into from
Sep 7, 2018

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Sep 6, 2018

The newly added label is sql_type, it has value: general and internal,
users can use this label to filter metrics they have interest.

Also, add keyword [INTERNAL] for restricted SQL printed in slow log for
convenient identification.

What problem does this PR solve?

Improve usability of TiDB. We output bunch of metrics for monitoring status of TiDB now, such as time spent on distsql. Sometimes users may notice high distsql time on dashboard, and they may wonder why my simple SQL is spending much time here, but actually, the monitored distsql time may belong to internal SQL queries(called restricted SQL) issued by DDL or auto Analyze for example. To avoid this confusion, we differentiate restricted SQL and general SQL in metrics, users can choose to only reflect metrics of general SQL on dashboard now, or they can monitor restricted SQL status as well.

What is changed and how it works?

add label for:

  • DistSQLQueryHistgram
  • QueryDurationHistogram
  • SessionExecuteParseDuration
  • SessionExecuteCompileDuration
  • SessionExecuteRunDuration

Also, add keyword [INTERNAL] for restricted SQL printed in slow log for convenient identification.

Check List

Tests

  • Unit test

  • Manual test (add detailed scripts or steps below)

    deploy a TiDB instance with pushgateway, prometheus and grafana for monitor(see also https://github.com/pingcap/tidb-ansible for instructions), run some workload on TiDB instance, then monitor status in grafana using the newly added label sql_type for filtering.

Related changes

  • Need to be included in the release note: add different labels for restricted SQL and general SQL in metrics

@shenli shenli added component/metrics type/enhancement The issue or PR belongs to an enhancement. labels Sep 6, 2018
Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Sep 6, 2018

@shenli @coocood PTAL

@coocood
Copy link
Member

coocood commented Sep 6, 2018

I think for end user, Internal may be better than Retricted.

The newly added label is `sql_type`, it has value: `general` and `internal`,
users can use this label to filter metrics they have interest.

Also, add keyword `[INTERNAL]` for restricted SQL printed in slow log for
convenient identification.
@eurekaka eurekaka force-pushed the restricted_metric_log branch from 6e12248 to 2689501 Compare September 6, 2018 12:12
@eurekaka eurekaka added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 6, 2018
@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 6, 2018

/run-all-tests

@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 6, 2018

@coocood updated to internal

@shenli
Copy link
Member

shenli commented Sep 6, 2018

@coocood PTAL

@coocood
Copy link
Member

coocood commented Sep 7, 2018

LGTM

@eurekaka eurekaka merged commit 8c44f56 into pingcap:master Sep 7, 2018
@eurekaka eurekaka deleted the restricted_metric_log branch September 7, 2018 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/metrics status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants