Skip to content

slack-vitess-r14.0.5: backport vitessio/vitess#12394#82

Merged
tanjinx merged 2 commits intoslack-vitess-r14.0.5from
pr-12394-slack-vitess-r14.0.5
May 11, 2023
Merged

slack-vitess-r14.0.5: backport vitessio/vitess#12394#82
tanjinx merged 2 commits intoslack-vitess-r14.0.5from
pr-12394-slack-vitess-r14.0.5

Conversation

@timvaillancourt
Copy link

Emit per workload labels for existing per table vttablet metrics (vitessio#12394)

  • Emit per workload labels for existing per table vttablet metrics

This adds the possibility to configure vttablet (via CLI flag) to also have a workload label for existing per table metrics (query counts, query times, query errors, query rows affected, query rows returned, query error counts). Workload can be any string that makes sense for the client application. For example, API endpoint name, controller, batch job name, application name or something else.

This is usefult to be able to gain observability about how the query load is distributed across different workloads.

This is achieved with two new CLI flags, namely:

  • enable-per-workload-table-metrics: whether to enable or disable per workload metric collection - disabled by default to preserve the current behavior, thus making the new feature opt-in only.
  • workload-label: a string to look for in query comments to identify the workload running the current query.

The workload is obtained by parsing query comments of the form:

/* ... <workload_label>=<workload_name>; ... */

For example, if vttablet is started with

--enable-per-workload-table-metrics --workload-label app_name

anda query is issued with a comment like

/* ... app_name=shop; ... */

then metrics will look like

vttablet_query_counts{plan="Select",table="dual", workload="shop"} 15479

instead of

vttablet_query_counts{plan="Select",table="dual"} 15479

Query comment parsing only takes place if --enable-per-workload-table-metrics is used, as to not incur parsing performance impact if the user does not want per workload metrics.

  • make linter happy

  • fix flags e2e test

  • Address PR comments:

  • Obtain workload information on the vtgate instead of the vttablet, avoiding double parsing.

  • Treat workload name as a query directive.

  • Send workload name from vtgate to vttablet as ExecuteOptions.

Additionally, annotate tabletserver's execution span with the workload name to also enrich traces with workload name data, in addition to metrics.

  • A few fixes:
  1. Rebuild some files with make proto.
  2. Protect against nil ExecuteOptions on the tabletserver.
  • Fix flags e2e test

  • Address PR comments

  • Fixes

  • Fix a comment

  • Fix e2e flag test

  • Update JS code for protobuf changes.

  • Fix QueryEngine unit test

  • Fix e2e flag test

  • Fix spurious tab in comment

  • Address PR comment

Don't use dual format flag for new flags - stick with - separated ones.


Description

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

…essio#12394)

* Emit per workload labels for existing per table vttablet metrics

This adds the possibility to configure vttablet (via CLI flag) to also have a
workload label for existing per table metrics (query counts, query times, query
errors, query rows affected, query rows returned, query error counts). Workload
can be any string that makes sense for the client application. For example, API
endpoint name, controller, batch job name, application name or something else.

This is usefult to be able to gain observability about how the query load is
distributed across different workloads.

This is achieved with two new CLI flags, namely:

* `enable-per-workload-table-metrics`: whether to enable or disable per
  workload metric collection - disabled by default to preserve the current
  behavior, thus making the new feature opt-in only.
* `workload-label`: a string to look for in query comments to identify the
  workload running the current query.

The workload is obtained by parsing query comments of the form:

/* ... <workload_label>=<workload_name>; ... */

For example, if vttablet is started with

`--enable-per-workload-table-metrics --workload-label app_name`

anda query is issued with a comment like

/* ... app_name=shop; ... */

then metrics will look like

```
vttablet_query_counts{plan="Select",table="dual", workload="shop"} 15479
```

instead of

```
vttablet_query_counts{plan="Select",table="dual"} 15479
```

Query comment parsing only takes place if `--enable-per-workload-table-metrics`
is used, as to not incur parsing performance impact if the user does not want
per workload metrics.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* make linter happy

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* fix flags e2e test

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Address PR comments:

* Obtain workload information on the vtgate instead of the vttablet, avoiding
  double parsing.
* Treat workload name as a query directive.
* Send workload name from vtgate to vttablet as ExecuteOptions.

Additionally, annotate tabletserver's execution span with the workload name
to also enrich traces with workload name data, in addition to metrics.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* A few fixes:

1. Rebuild some files with `make proto`.
2. Protect against nil ExecuteOptions on the tabletserver.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix flags e2e test

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Address PR comments

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fixes

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix a comment

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix e2e flag test

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Update JS code for protobuf changes.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix QueryEngine unit test

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix e2e flag test

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Fix spurious tab in comment

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

* Address PR comment

Don't use dual format flag for new flags - stick with - separated ones.

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>

---------

Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
@timvaillancourt timvaillancourt requested a review from a team as a code owner May 11, 2023 15:49

// WorkloadName specifies the name of the workload as indicated in query directives. This is used for instrumentation
// in metrics and tracing spans.
string WorkloadName = 15;
Copy link
Author

@timvaillancourt timvaillancourt May 11, 2023

Choose a reason for hiding this comment

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

Note: 15 is used because 13 and 14 are reserved by new fields introduced in v15+v16, and this PR was originally made to v16/v17

@timvaillancourt timvaillancourt added v14 upstream-backport An upstream backport labels May 11, 2023
Copy link

@tanjinx tanjinx left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Tim!

@tanjinx tanjinx merged commit 49190df into slack-vitess-r14.0.5 May 11, 2023
@timvaillancourt timvaillancourt deleted the pr-12394-slack-vitess-r14.0.5 branch May 12, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upstream-backport An upstream backport v14

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants