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

slack-vitess-r14.0.4-dsdefense branch: initial cherry-picks, part 1 #54

Conversation

timvaillancourt
Copy link
Member

@timvaillancourt timvaillancourt commented Mar 14, 2023

Description

This PR adds the following upstream cherry-picks to slack-vitess-r14.0.4-dsdefense:

  1. Allow override of build git env in docker/base builds vitessio/vitess#11968
  2. Add basic metrics to vttablet transaction throttler vitessio/vitess#12418
  3. Add CpuUsage to RealtimeStats vitessio/vitess#12389 (in-review upstream, so I merged-in every commit)
    • sync/atomic usage adapted to use vitess.io/vitess/go/sync2 because it requires Go >= 1.19
  4. Emit per workload labels for existing per table vttablet metrics vitessio/vitess#12394
  5. Add priority support to transaction throttler vitessio/vitess#12662

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

timvaillancourt and others added 16 commits March 2, 2023 00:21
* Pass BUILD_GIT_BRANCH and BUILD_GIT_REV env into docker build

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix bootstrap version

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix bootstrap version, again

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix ws

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
* Add basic stats to vttablet tx throttler

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* test new metrics

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* reorder

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* short names

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Add max rate

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Move NewGaugeFunc to under conditional

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Use env

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Remove env from TxThrottler struct

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix tests

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* PR suggestion

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Fix unit test

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* reorder test vars

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

---------

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@timvaillancourt timvaillancourt force-pushed the slack-vitess-r14.0.4-dsdefense-cp1 branch from f978969 to 34f24da Compare March 22, 2023 19:30
…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>
…pect that

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>
Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com>
@timvaillancourt
Copy link
Member Author

CI crashing on:

runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:220 (0x44c69c)
runtime/signal_unix.go:818 (0x44c66c)
vitess.io/vitess/go/stats/counters.go:196 (0x8b6239)
vitess.io/vitess/go/vt/vttablet/tabletserver/query_engine.go:464 (0x13f0ef7)
vitess.io/vitess/go/vt/vttablet/tabletserver/query_executor.go:124 (0x13f3299)
vitess.io/vitess/go/vt/vttablet/tabletserver/query_executor.go:172 (0x13f2a1b)
vitess.io/vitess/go/vt/vttablet/tabletserver/tabletserver.go:763 (0x140d80e)
vitess.io/vitess/go/vt/vttablet/tabletserver/tabletserver.go:1318 (0x14121db)
vitess.io/vitess/go/vt/vttablet/tabletserver/tabletserver.go:731 (0x140d464)
vitess.io/vitess/go/vt/vtcombo/tablet_map.go:466 (0x1521395)
vitess.io/vitess/go/vt/vttablet/queryservice/wrapped.go:185 (0xd85288)
vitess.io/vitess/go/vt/vtgate/tabletgateway.go:329 (0x1e6ff63)
vitess.io/vitess/go/vt/vttablet/queryservice/wrapped.go:183 (0xd85170)
vitess.io/vitess/go/vt/vtgate/schema/tracker.go:87 (0x1e46c51)
vitess.io/vitess/go/vt/vtgate/schema/tracker.go:242 (0x1e48896)
vitess.io/vitess/go/vt/vtgate/vtgate.go:340 (0x1e824ee)
vitess.io/vitess/go/vt/vtgate/vtgate.go:321 (0x1e821c4)
vitess.io/vitess/go/vt/vtgate/vtgate.go:219 (0x1e81587)
vitess.io/vitess/go/cmd/vtcombo/main.go:255 (0x1e9070f)
runtime/proc.go:250 (0x438991)
runtime/asm_amd64.s:1571 (0x4694a0)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x1e48051]

So it is crashing here, which is close to changes made to .AddStats(...) but I can't seem to see why 🤷

Copy link
Collaborator

@ejortegau ejortegau left a comment

Choose a reason for hiding this comment

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

LGTM

@timvaillancourt timvaillancourt merged commit 8878f5b into slack-vitess-r14.0.4-dsdefense Mar 23, 2023
@timvaillancourt timvaillancourt deleted the slack-vitess-r14.0.4-dsdefense-cp1 branch March 23, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants