Skip to content

log: report scheduling group along with shard id - #1666

Merged
xemul merged 2 commits into
scylladb:masterfrom
tchaikov:report-sched-group
Aug 3, 2023
Merged

log: report scheduling group along with shard id#1666
xemul merged 2 commits into
scylladb:masterfrom
tchaikov:report-sched-group

Conversation

@tchaikov

@tchaikov tchaikov commented May 23, 2023

Copy link
Copy Markdown
Contributor

The scheduling group provides useful information like which tenant is
currently executing, and is inexpensive to provide, so log it with every
message. because the names of scheduling group could be too long
to be printed in every single logging message, a shortened version is
used. to avoid the complexity of communicating the width / max length
of the shortened name between logging and reactor subsystems, a
fixed-length string_view is returned. seastar::sstring or std::string
is too heavy-weighted for this use case.

we shorten the names with two strategies

  • if the name contains underscore, the shortened name is translated
    using following pseudo code:
    ''.join(part[0] for part in name.split("_", maxsplit=3))
    for instance, the short name of "memtable_to_cache" would be "mc"
  • if the name does not contain underscore, the shortened name would
    be the first 3 characters of the full name, so the short name of
    "streaming" would be "str".

@tchaikov

Copy link
Copy Markdown
Contributor Author

the logging messages now look like:

INFO  2023-05-23 15:13:33,902 [shard  0:n/a] seastar - Perf-based stall detector creation failed (EACCESS), try setting /proc/sys/kernel/perf_event_paranoid to 1 or less to enable kernel backtraces: falling back to posix timer.
INFO  2023-05-23 15:13:33,906 [shard  0:mai] seastar - Created fair group io-queue-0 for 16 queues, capacity rate 2147483:2147483, limit 12582912, rate 16777216 (factor 1), threshold 2000, per tick grab 786432
INFO  2023-05-23 15:13:33,907 [shard  0:mai] seastar - IO queue uses 0.75ms latency goal for device 0
INFO  2023-05-23 15:13:33,907 [shard  0:mai] seastar - Created io group dev(0), length limit 4194304:4194304, rate 2147483647:2147483647
INFO  2023-05-23 15:13:33,907 [shard  0:mai] seastar - Created io queue dev(0) capacities: 512:2000:2000 1024:3000:3000 2048:5000:5000 4096:9000:9000 8192:17000:17000 16384:33000:33000 32768:65000:65000 65536:129000:129000 131072:257000:257000

@xemul

xemul commented May 23, 2023

Copy link
Copy Markdown
Contributor

Sched groups we now have in scylla:

$ cat main.cc | fgrep make_sched_group | sed -e 's/^.*(//' -e 's/,.*//' -e 's/"//g' | fgrep -v 'sstring' | sort
background_reclaim
compaction
gossip
mem_compaction
memtable
memtable_to_cache
statement
streaming

their short 3-char names map to

bac
com
gos
mem
mem
mem
sta
str

and that's too short, unfortunately :(

There can be several ways to solve it:

  • make short-name-prefix-length app-configurable
  • extend the sched group creation API with the short-name-for-logging alias argument
  • make the algo check if new group's short name matches one of the existing and pick up other letter(s) for the newcomer
  • feed the list to ChatGPT (or any other AI of the choice) on start asking it to shorten the list to unique 3-chars-wide one

I prefer the last one, don't insist

@mykaul

mykaul commented May 23, 2023

Copy link
Copy Markdown
Contributor

Why are we fixed on 3 letters only?

@xemul

xemul commented May 23, 2023

Copy link
Copy Markdown
Contributor

Why are we fixed on 3 letters only?

@mykaul , because screen is to narrow for more

@mykaul

mykaul commented May 23, 2023

Copy link
Copy Markdown
Contributor

shard 0

I think we can drop the 'a', as everyone understands 'shrd' is shard! and just like that we got ourselves another letter! ;-)

(What amazes me more is that we have code to shorten the name, instead of a const array of strings... ? @tchaikov - isn't that a bit of an overkill?)

@tchaikov

tchaikov commented May 23, 2023

Copy link
Copy Markdown
Contributor Author

Sched groups we now have in scylla:

$ cat main.cc | fgrep make_sched_group | sed -e 's/^.*(//' -e 's/,.*//' -e 's/"//g' | fgrep -v 'sstring' | sort
background_reclaim
compaction
gossip
mem_compaction
memtable
memtable_to_cache
statement
streaming

their short 3-char names map to

bac
com
gos
mem
mem
mem
sta
str

with the change proposed by this PR, the ones has "_" in their name are mapped like (i am including memtable here so we can compare them side-by-side:

mem_compaction =>
  mc
memtable =>
  mem
memtable_to_cache =>
  mtc

and that's too short, unfortunately :(

There can be several ways to solve it:

* make short-name-prefix-length app-configurable

* extend the sched group creation API with the short-name-for-logging alias argument

* make the algo check if new group's short name matches one of the existing and pick up other letter(s) for the newcomer

we can do this. but it's even more overkill than the existing implementation. but since with the

''.join(part[0] for part in name.split("_", maxsplit=3))

i guess we (scylla at least) are safe at this moment, am i right?

* feed the list to ChatGPT (or any other AI of the choice) on start asking it to shorten the list to unique 3-chars-wide one

lol.

I prefer the last one, don't insist

@tchaikov

Copy link
Copy Markdown
Contributor Author

shard 0

I think we can drop the 'a', as everyone understands 'shrd' is shard! and just like that we got ourselves another letter! ;-)

(What amazes me more is that we have code to shorten the name, instead of a const array of strings... ? @tchaikov - isn't that a bit of an overkill?)

because i don't want to hardwire to a set of pre-configured name. and wanted to handle the case like "hello__world" (with two consecutive underscores).

@xemul

xemul commented May 23, 2023

Copy link
Copy Markdown
Contributor

with the change proposed by this PR, the ones has "_" in their name are mapped like (i am including memtable here so we can compare them side-by-side:

Missed that 😳 my bad. Plz, add this to the PR's commit message/ So we have

br
com
gos
mc
mem
mtc
sta
str

Yes, that's unique, but quite ... fuzzy. With 4-letters we can have more descriptive (from my POV) names like

bgrc
comp
gssp
mcmp
mtbl
mtoc
stmt
strm

but it needs optional make-sched-group API argument. Oh, well...

@xemul

xemul commented May 23, 2023

Copy link
Copy Markdown
Contributor

shard 0

I think we can drop the 'a', as everyone understands 'shrd' is shard! and just like that we got ourselves another letter! ;-)

Grt ctch! W cn drp ll th vwls frm lgs. t wll stll b rdbl, bt spc svng ttl wrth t

@tchaikov

Copy link
Copy Markdown
Contributor Author

with the change proposed by this PR, the ones has "_" in their name are mapped like (i am including memtable here so we can compare them side-by-side:

Missed that flushed my bad. Plz, add this to the PR's commit message/ So we have

br
com
gos
mc
mem
mtc
sta
str

Yes, that's unique, but quite ... fuzzy. With 4-letters we can have more descriptive (from my POV) names like

bgrc
comp
gssp
mcmp
mtbl
mtoc
stmt
strm

yeah, agreed. and this reminds me the IATA codes .

but it needs optional make-sched-group API argument. Oh, well...

can we afford this? if yes, i can add it..

@mykaul

mykaul commented May 23, 2023

Copy link
Copy Markdown
Contributor

shard 0

I think we can drop the 'a', as everyone understands 'shrd' is shard! and just like that we got ourselves another letter! ;-)

Grt ctch! W cn drp ll th vwls frm lgs. t wll stll b rdbl, bt spc svng ttl wrth t

My next T-shirt - https://www.fivefingertees.com/products/intelligence-is-the-ability-to-adapt-to-change-t-shirt-1

@tchaikov
tchaikov force-pushed the report-sched-group branch from f8e86ea to f33cc32 Compare May 23, 2023 09:37
@tchaikov

Copy link
Copy Markdown
Contributor Author

changelog

  • update commit message with examples of short names.

@tchaikov

Copy link
Copy Markdown
Contributor Author

@avikivity @xemul ping for reviews.

@xemul

xemul commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

My comment about the ability to specify short-name by hand API call (opt creation-time arg) still stands. I'm pretty confident that people will get pissed off by seeing br com gos mc mem mtc sta str prefixes in logs

@tchaikov
tchaikov force-pushed the report-sched-group branch from f33cc32 to 5f2e82b Compare June 15, 2023 08:23
@tchaikov

Copy link
Copy Markdown
Contributor Author

a sample output looks like

$  build/debug/demos/tutorial_examples_demo 
WARNING: debug mode. Not for benchmarking or production
INFO  2023-06-15 16:23:35,396 seastar - Reactor backend: io_uring
INFO  2023-06-15 16:23:35,397 [shard  0:n/a ] seastar - Perf-based stall detector creation failed (EACCESS), try setting /proc/sys/kernel/perf_event_paranoid to 1 or less to enable kernel backtraces: falling back to posix timer.
INFO  2023-06-15 16:23:35,398 [shard  0:main] seastar - Created fair group io-queue-0 for 16 queues, capacity rate 2147483:2147483, limit 12582912, rate 16777216 (factor 1), threshold 2000, per tick grab 786432
INFO  2023-06-15 16:23:35,399 [shard  0:main] seastar - IO queue uses 0.75ms latency goal for device 0
INFO  2023-06-15 16:23:35,399 [shard  0:main] seastar - Created io group dev(0), length limit 4194304:4194304, rate 2147483647:2147483647
INFO  2023-06-15 16:23:35,399 [shard  0:main] seastar - Created io queue dev(0) capacities: 512:2000:2000 1024:3000:3000 2048:5000:5000 4096:9000:9000 8192:17000:17000 16384:33000:33000 32768:65000:65000 65536:129000:129000 131072:257000:257000
This is the tutorial examples demo.  It is not running anything but rather makes sure the tutorial examples compile

@tchaikov
tchaikov force-pushed the report-sched-group branch 3 times, most recently from 6662cb6 to 6749f9a Compare June 15, 2023 08:44
@tchaikov

Copy link
Copy Markdown
Contributor Author

My comment about the ability to specify short-name by hand API call (opt creation-time arg) still stands. I'm pretty confident that people will get pissed off by seeing br com gos mc mem mtc sta str prefixes in logs

@xemul i added the APIs which accept the shortname. will update scylla's side, once this change is merged. and the seastar submodule is updated.

@tchaikov

tchaikov commented Jul 7, 2023

Copy link
Copy Markdown
Contributor Author

@xemul Pavel, ping?

@tchaikov

tchaikov commented Aug 1, 2023

Copy link
Copy Markdown
Contributor Author

@xemul Pavel, ping again?

Comment thread src/core/reactor.cc Outdated
if (new_shortname.empty()) {
_shortname = shorten_name(_name, shortname_size);
} else {
_shortname = new_shortname;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should it be in sync with the constructor and do format("{:>{}}", new_shortname, shortname_size) thing as well?

The scheduling group provides useful information like which tenant is
currently executing, and is inexpensive to provide, so log it with every
message. because the names of scheduling group could be too long
to be printed in every single logging message, a shortened version is
used. the shortname is specified when creating a scheduling group or
when renaming it. so the involved functions are updated so new variants
is added, these variants allows user to specify an optional shortname
for the scheduling group. if user continue using the old API, the
shortname is created automatically with two strategies

* if the name contains underscore, the shortened name is translated
  using following pseudo code:
  ```py
  ''.join(part[0] for part in name.split("_", maxsplit=4))
  ```
  for instance, the short name of "memtable_to_cache" would be "mc"
* if the name does not contain underscore, the shortened name would
  be the first 4 characters of the full name, so the short name of
  "streaming"  would be "str".

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
task_queue::rename() is used in its constructor, if the name is empty,
the task_queue won't get its new name or gets registered its stats
with metrics subsystem.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@tchaikov
tchaikov force-pushed the report-sched-group branch from 6749f9a to e10ca73 Compare August 3, 2023 07:20
@tchaikov

tchaikov commented Aug 3, 2023

Copy link
Copy Markdown
Contributor Author

v2:

  • reuse rename() in the ctor of task_queue to dedup the logic of setting name and shortname.

@xemul hi Pavel, thank you for your review. i just changed accordingly. could you take yet another look?

@xemul xemul closed this in 5978227 Aug 3, 2023
@xemul
xemul merged commit 5978227 into scylladb:master Aug 3, 2023
@tchaikov
tchaikov deleted the report-sched-group branch September 4, 2023 12:00
@tchaikov

tchaikov commented Sep 4, 2023

Copy link
Copy Markdown
Contributor Author

the corresponding scylla issue: scylladb/scylladb#15267

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.

3 participants