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

ui,admission: observability improvements for admission control #68595

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

sumeerbhola
Copy link
Collaborator

  • Trace statements for latency incurred in admission queues.
  • Certain admission control metrics are now included in the
    overload dashboard. Specifically,
    • Resource bottlenecks can be identified using the
      "KV Admission Slots" and "KV Admission IO Tokens Exhausted
      Duration Per Second" graphs.
    • The rate at which admission control is admitting requests
      is in the "Admission Work Rate" graphs and the corresponding
      latency rate (for all requests) is in
      "Admission Latency Rate". Dividing the latter by the former
      gives the mean admission latency.
    • The 75th percentile latency for those requests that actually
      waited for admission is in the
      "Admission Latency: 75th percentile" graph.
      When admission control is off most of these graphs will be
      empty or zero, and the total KV admission slots will be 1.

Informs #65955

Release note (ui change): admission control metrics are added to
Overload dashboard.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@sumeerbhola
Copy link
Collaborator Author

@dhartunian I don't know what to make of the lint failure -- I did not add a Tooltip. Could you help?

[16:00:20]	    console.error
[16:00:20]	      Warning: An update to Tooltip inside a test was not wrapped in act(...).
[16:00:20]	      
[16:00:20]	      When testing, code that causes React state updates should be wrapped into act(...):
[16:00:20]	      
[16:00:20]	      act(() => {
[16:00:20]	        /* fire events that update state */
[16:00:20]	      });
[16:00:20]	      /* assert on the output */
[16:00:20]	      
[16:00:20]	      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
[16:00:20]	          in Tooltip
[16:00:20]	          in td
[16:00:20]	          in Unknown
[16:00:20]	          in tr
[16:00:20]	          in Unknown (created by StatementsSortedTable)
[16:00:20]	          in tbody (created by StatementsSortedTable)
[16:00:20]	          in table (created by StatementsSortedTable)
[16:00:20]	          in div (created by StatementsSortedTable)
[16:00:20]	          in StatementsSortedTable
[16:00:20]	          in section
[16:00:20]	          in div
[16:00:20]	          in Unknown (created by StatementsPage)
[16:00:20]	          in div (created by StatementsPage)
[16:00:20]	          in StatementsPage
[16:00:20]	          in Router (created by MemoryRouter)
[16:00:20]	          in MemoryRouter (created by WrapperComponent)
[16:00:20]	          in WrapperComponent
[16:00:20]	
[16:00:20]	      at warningWithoutStack (node_modules/react-dom/cjs/react-dom.development.js:530:32)
[16:00:20]	      at warnIfNotCurrentlyActingUpdatesInDEV (node_modules/react-dom/cjs/react-dom.development.js:25848:7)
[16:00:20]	      at dispatchAction (node_modules/react-dom/cjs/react-dom.development.js:17072:9)
[16:00:20]	      at fn (node_modules/@cockroachlabs/ui-components/dist/main.js:25421:9)
[16:00:20]	      at Object.forceUpdate (node_modules/@cockroachlabs/ui-components/dist/main.js:365:21)
[16:00:20]	      at node_modules/@cockroachlabs/ui-components/dist/main.js:378:20
[16:00:20]	      at node_modules/@cockroachlabs/ui-components/dist/main.js:377:16

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

I think we'll want to do a review of all new admission-control-related graphs before release, together with PM

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @sumeerbhola)


pkg/ui/src/views/cluster/containers/nodeGraphs/dashboards/overload.tsx, line 149 at r1 (raw file):

    <LineGraph title="Admission Latency Rate" sources={nodeSources}>
      <Axis label="latency rate (micros/sec)">

This is pretty obscure to be looking at directly.. It's hard to reason about what this rate means. I guess it is not possible to show the wait_sum / admitted graph without calculating a new metric?


pkg/ui/src/views/cluster/containers/nodeGraphs/dashboards/overload.tsx, line 179 at r1 (raw file):

    <LineGraph title="Admission Latency: 75th percentile" sources={nodeSources}>
      <Axis label="latency for requests that waited (nanos)">

"latency" can be confusing (could refer to the overall execution latency). Maybe admission delay or wait time?

@nathanstilwell
Copy link
Contributor

@sumeerbhola Filling in for @dhartunian, the several warnings about using act(...) around Tooltips are unfortunately a distraction from the actual error which is,

    $ eslint './{src,ccl}/**/*.{tsx,ts,js}'
15:58:12     [BABEL] Note: The code generator has deoptimised the styling of /go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/nise/nise.js as it exceeds the max of 500KB.
15:58:15     [BABEL] Note: The code generator has deoptimised the styling of /go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/react-dom/cjs/react-dom.development.js as it exceeds the max of 500KB.
15:58:27     
15:58:27     /go/src/github.com/cockroachdb/cockroach/pkg/ui/src/views/cluster/containers/nodeGraphs/dashboards/overload.tsx
15:58:27       105:15  error  Replace `·title="KV·Admission·IO·Tokens·Exhausted·Duration·Per·Second"·sources={nodeSources}` with `⏎······title="KV·Admission·IO·Tokens·Exhausted·Duration·Per·Second"⏎······sources={nodeSources}⏎····`  prettier/prettier
15:58:27       206:18  error  Delete `⏎`                                                                                                                                                                                                 prettier/prettier
15:58:27     
15:58:27     ✖ 2 problems (2 errors, 0 warnings)
15:58:27       2 errors and 0 warnings potentially fixable with the `--fix` option.

We use a code formatter called Prettier as a plugin for our linter Eslint. Often folks working with React will have tools builtin to handle the formatting automatically. We do have a yarn command to auto-format files, but I'm sorry to say it is not something that is a part of the regular build in make. After making React/TypeScript/JavaScript changes, the following should remove these kinds of errors,

# from the root of cockroachdb repo
>  yarn --cwd pkg/ui run lint:fix
yarn run v1.22.10
$ eslint './{src,ccl}/**/*.{tsx,ts,js}' --fix
✨  Done in 17.28s.

# to view any lint error on the front-end (you can also use `make ui-lint`) 
>  yarn --cwd pkg/ui run lint
yarn run v1.22.10
$ eslint './{src,ccl}/**/*.{tsx,ts,js}'
✨  Done in 17.73s.

Please let me know if you run into any issues or if I can provide more clarification.

- Trace statements for latency incurred in admission queues.
- Certain admission control metrics are now included in the
  overload dashboard. Specifically,
  - Resource bottlenecks can be identified using the
    "KV Admission Slots" and "KV Admission IO Tokens Exhausted
    Duration Per Second" graphs.
  - The rate at which admission control is admitting requests
    is in the "Admission Work Rate" graphs and the corresponding
    delay rate (for all requests) is in
    "Admission Delay Rate". Dividing the latter by the former
    gives the mean admission delay.
  - The 75th percentile delay for those requests that actually
    waited for admission is in the
    "Admission Delay: 75th percentile" graph.
  When admission control is off most of these graphs will be
  empty or zero, and the total KV admission slots will be 1.

Informs cockroachdb#65955

Release note (ui change): admission control metrics are added to
Overload dashboard.
@sumeerbhola
Copy link
Collaborator Author

TFTRs @RaduBerinde @nathanstilwell !

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 19, 2021

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.

4 participants