{"thread":{"id":"60015","subject":"[PATCH 1/2] trace2: fix a comment","startedAt":"2023-07-19T23:35:47Z","lastAt":"2023-08-07T18:25:17Z","messageCount":9,"participants":["Beat Bolli","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479665","messageId":"20230719232444.555838-1-dev+git@drbeat.li","threadId":"60015","inReplyTo":null,"subject":"[PATCH 1/2] trace2: fix a comment","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2023-07-19T23:24:43Z","receivedAt":"2023-07-19T23:35:47Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"When the trace2 counter mechanism was added in 81071626ba (trace2: add\nglobal counter mechanism, 2022-10-24), the name of the file where new\ncounters are added was misspelled in a comment.\n\nUse the correct file name.\n\nSigned-off-by: Beat Bolli \n---\n trace2.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/trace2.h b/trace2.h\nindex f5c5a9e6bac5..64c747c1df1b 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -541,7 +541,7 @@ void trace2_timer_stop(enum trace2_timer_id tid);\n * elsewhere as array indexes).\n *\n * Any values added to this enum be also be added to the\n- * `tr2_counter_metadata[]` in `trace2/tr2_tr2_ctr.c`.\n+ * `tr2_counter_metadata[]` in `trace2/tr2_ctr.c`.\n */\n enum trace2_counter_id {\n \t/*\n-- \n2.41.0\n\n"},{"id":"479666","messageId":"20230719232444.555838-2-dev+git@drbeat.li","threadId":"60015","inReplyTo":"20230719232444.555838-1-dev+git@drbeat.li","subject":"[PATCH 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2023-07-19T23:24:44Z","receivedAt":"2023-07-19T23:36:03Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"As mentioned in the subthread starting at [1], trace2 counters should be\nused to count events instead of ad-hoc static variables.\n\nConvert the static variables that count fsync calls to trace2 counters,\nreducing the coupling between wrapper.c and the trace2 subsystem.\n\nThe counters are not per-thread because the ones being replaced also\nwere not.\n\n[1] https://lore.kernel.org/git/20230627195251.1973421-2-calvinwan@google.com/\n\nSigned-off-by: Beat Bolli \n---\nI have based this series on master, so this patch will create a trivial\nmerge conflict with c489f47a649d (refs/packed-backend.c: add trace2\ncounters for jump list, 2023-07-10) on next, which also adds a new\ncounter.\n\n trace2.c | 1 -\n trace2.h | 4 ++++\n trace2/tr2_ctr.c | 10 ++++++++++\n wrapper.c | 19 ++-----------------\n wrapper.h | 5 -----\n 5 files changed, 16 insertions(+), 23 deletions(-)\n\ndiff --git a/trace2.c b/trace2.c\nindex 49c23bfd05a7..6dc74dff4c73 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -276,7 +276,6 @@ void trace2_cmd_exit_fl(const char *file, int line, int code)\n \tif (!trace2_enabled)\n \t\treturn;\n \n-\ttrace_git_fsync_stats();\n \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);\n \n \ttr2main_exit_code = code;\ndiff --git a/trace2.h b/trace2.h\nindex 64c747c1df1b..12211d3bd61b 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -552,6 +552,10 @@ enum trace2_counter_id {\n \tTRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */\n \tTRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */\n \n+\t/* counts number of fsyncs */\n+\tTRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,\n+\tTRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH,\n+\n \t/* Add additional counter definitions before here. */\n \tTRACE2_NUMBER_OF_COUNTERS\n };\ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex b342d3b1a3c0..14a082651001 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -27,6 +27,16 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n \t\t.name = \"test2\",\n \t\t.want_per_thread_events = 1,\n \t},\n+\t[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {\n+\t\t.category = \"fsync\",\n+\t\t.name = \"writeout_only\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n+\t[TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH] = {\n+\t\t.category = \"fsync\",\n+\t\t.name = \"hardware_flush\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n \n \t/* Add additional metadata before here. */\n };\ndiff --git a/wrapper.c b/wrapper.c\nindex 22be9812a724..dea54a326073 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -10,9 +10,6 @@\n #include \"strbuf.h\"\n #include \"trace2.h\"\n \n-static intmax_t count_fsync_writeout_only;\n-static intmax_t count_fsync_hardware_flush;\n-\n #ifdef HAVE_RTLGENRANDOM\n /* This is required to get access to RtlGenRandom. */\n #define SystemFunction036 NTAPI SystemFunction036\n@@ -551,7 +548,7 @@ int git_fsync(int fd, enum fsync_action action)\n {\n \tswitch (action) {\n \tcase FSYNC_WRITEOUT_ONLY:\n-\t\tcount_fsync_writeout_only += 1;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY, 1);\n \n #ifdef __APPLE__\n \t\t/*\n@@ -583,7 +580,7 @@ int git_fsync(int fd, enum fsync_action action)\n \t\treturn -1;\n \n \tcase FSYNC_HARDWARE_FLUSH:\n-\t\tcount_fsync_hardware_flush += 1;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH, 1);\n \n \t\t/*\n \t\t * On macOS, a special fcntl is required to really flush the\n@@ -600,18 +597,6 @@ int git_fsync(int fd, enum fsync_action action)\n \t}\n }\n \n-static void log_trace_fsync_if(const char *key, intmax_t value)\n-{\n-\tif (value)\n-\t\ttrace2_data_intmax(\"fsync\", the_repository, key, value);\n-}\n-\n-void trace_git_fsync_stats(void)\n-{\n-\tlog_trace_fsync_if(\"fsync/writeout-only\", count_fsync_writeout_only);\n-\tlog_trace_fsync_if(\"fsync/hardware-flush\", count_fsync_hardware_flush);\n-}\n-\n static int warn_if_unremovable(const char *op, const char *file, int rc)\n {\n \tint err;\ndiff --git a/wrapper.h b/wrapper.h\nindex c85b1328d163..79a9c1b5077b 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -87,11 +87,6 @@ enum fsync_action {\n */\n int git_fsync(int fd, enum fsync_action action);\n \n-/*\n- * Writes out trace statistics for fsync using the trace2 API.\n- */\n-void trace_git_fsync_stats(void);\n-\n /*\n * Preserves errno, prints a message, but gives no warning for ENOENT.\n * Returns 0 on success, which includes trying to unlink an object that does\n-- \n2.41.0\n\n"},{"id":"479667","messageId":"xmqqbkg75tkm.fsf@gitster.g","threadId":"60015","inReplyTo":"20230719232444.555838-2-dev+git@drbeat.li","subject":"Re: [PATCH 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T00:12:57Z","receivedAt":"2023-07-20T00:13:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Beat Bolli writes:\n\n> As mentioned in the subthread starting at [1], trace2 counters should be\n> used to count events instead of ad-hoc static variables.\n>\n> Convert the static variables that count fsync calls to trace2 counters,\n> reducing the coupling between wrapper.c and the trace2 subsystem.\n>\n> The counters are not per-thread because the ones being replaced also\n> were not.\n>\n> [1] https://lore.kernel.org/git/20230627195251.1973421-2-calvinwan@google.com/\n>\n> Signed-off-by: Beat Bolli \n> ---\n> I have based this series on master, so this patch will create a trivial\n> merge conflict with c489f47a649d (refs/packed-backend.c: add trace2\n> counters for jump list, 2023-07-10) on next, which also adds a new\n> counter.\n\nThanks for leaving a note. This one was trivial enough to resolve,\nbut it is a good discipline to always make trial merges to 'next'\nand with other topics in flight.\n\nDid t5351 pass for you with this patch? Any other test breakages\nthat the patch needs to also adjust?\n\nThanks.\n"},{"id":"479671","messageId":"20230720164823.625815-1-dev+git@drbeat.li","threadId":"60015","inReplyTo":"xmqqbkg75tkm.fsf@gitster.g","subject":"[PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2023-07-20T16:48:23Z","receivedAt":"2023-07-20T16:48:56Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"As mentioned in the thread starting at [1], trace2 counters should be\nused to count events instead of ad-hoc static variables.\n\nConvert the two fsync static variables to trace2 counters, reducing the\ncoupling between wrapper.c and the trace2 subsystem. Adjust t/t5351 to\nmatch the trace2 counter output format.\n\nThe counters are not per-thread because the ones being replaced also\nwere not.\n\n[1] https://lore.kernel.org/git/20230627195251.1973421-2-calvinwan@google.com/\n\nSigned-off-by: Beat Bolli \n---\nv2:\n- Adjust t/t5351\n- Update commit message\n\n t/t5351-unpack-large-objects.sh | 6 +++---\n trace2.c | 1 -\n trace2.h | 4 ++++\n trace2/tr2_ctr.c | 10 ++++++++++\n wrapper.c | 19 ++-----------------\n wrapper.h | 5 -----\n 6 files changed, 19 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh\nindex 8c8af99b844b..43cbcd5d497e 100755\n--- a/t/t5351-unpack-large-objects.sh\n+++ b/t/t5351-unpack-large-objects.sh\n@@ -55,7 +55,7 @@ check_fsync_events () {\n \n \tcat >expect &&\n \tsed -n \\\n-\t\t-e '/^{\"event\":\"data\",.*\"category\":\"fsync\",/ {\n+\t\t-e '/^{\"event\":\"counter\",.*\"category\":\"fsync\",/ {\n \t\t\ts/.*\"category\":\"fsync\",//;\n \t\t\ts/}$//;\n \t\t\tp;\n@@ -78,8 +78,8 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '\n \t\tflush_count=1\n \tfi &&\n \tcheck_fsync_events trace2.txt <<-EOF &&\n-\t\"key\":\"fsync/writeout-only\",\"value\":\"6\"\n-\t\"key\":\"fsync/hardware-flush\",\"value\":\"$flush_count\"\n+\t\"name\":\"writeout-only\",\"count\":6\n+\t\"name\":\"hardware-flush\",\"count\":$flush_count\n \tEOF\n \n \ttest_dir_is_empty dest.git/objects/pack &&\ndiff --git a/trace2.c b/trace2.c\nindex 49c23bfd05a7..6dc74dff4c73 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -276,7 +276,6 @@ void trace2_cmd_exit_fl(const char *file, int line, int code)\n \tif (!trace2_enabled)\n \t\treturn;\n \n-\ttrace_git_fsync_stats();\n \ttrace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);\n \n \ttr2main_exit_code = code;\ndiff --git a/trace2.h b/trace2.h\nindex 64c747c1df1b..12211d3bd61b 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -552,6 +552,10 @@ enum trace2_counter_id {\n \tTRACE2_COUNTER_ID_TEST1 = 0, /* emits summary event only */\n \tTRACE2_COUNTER_ID_TEST2, /* emits summary and thread events */\n \n+\t/* counts number of fsyncs */\n+\tTRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY,\n+\tTRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH,\n+\n \t/* Add additional counter definitions before here. */\n \tTRACE2_NUMBER_OF_COUNTERS\n };\ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex b342d3b1a3c0..6491d25396e0 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -27,6 +27,16 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n \t\t.name = \"test2\",\n \t\t.want_per_thread_events = 1,\n \t},\n+\t[TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY] = {\n+\t\t.category = \"fsync\",\n+\t\t.name = \"writeout-only\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n+\t[TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH] = {\n+\t\t.category = \"fsync\",\n+\t\t.name = \"hardware-flush\",\n+\t\t.want_per_thread_events = 0,\n+\t},\n \n \t/* Add additional metadata before here. */\n };\ndiff --git a/wrapper.c b/wrapper.c\nindex 22be9812a724..dea54a326073 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -10,9 +10,6 @@\n #include \"strbuf.h\"\n #include \"trace2.h\"\n \n-static intmax_t count_fsync_writeout_only;\n-static intmax_t count_fsync_hardware_flush;\n-\n #ifdef HAVE_RTLGENRANDOM\n /* This is required to get access to RtlGenRandom. */\n #define SystemFunction036 NTAPI SystemFunction036\n@@ -551,7 +548,7 @@ int git_fsync(int fd, enum fsync_action action)\n {\n \tswitch (action) {\n \tcase FSYNC_WRITEOUT_ONLY:\n-\t\tcount_fsync_writeout_only += 1;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_FSYNC_WRITEOUT_ONLY, 1);\n \n #ifdef __APPLE__\n \t\t/*\n@@ -583,7 +580,7 @@ int git_fsync(int fd, enum fsync_action action)\n \t\treturn -1;\n \n \tcase FSYNC_HARDWARE_FLUSH:\n-\t\tcount_fsync_hardware_flush += 1;\n+\t\ttrace2_counter_add(TRACE2_COUNTER_ID_FSYNC_HARDWARE_FLUSH, 1);\n \n \t\t/*\n \t\t * On macOS, a special fcntl is required to really flush the\n@@ -600,18 +597,6 @@ int git_fsync(int fd, enum fsync_action action)\n \t}\n }\n \n-static void log_trace_fsync_if(const char *key, intmax_t value)\n-{\n-\tif (value)\n-\t\ttrace2_data_intmax(\"fsync\", the_repository, key, value);\n-}\n-\n-void trace_git_fsync_stats(void)\n-{\n-\tlog_trace_fsync_if(\"fsync/writeout-only\", count_fsync_writeout_only);\n-\tlog_trace_fsync_if(\"fsync/hardware-flush\", count_fsync_hardware_flush);\n-}\n-\n static int warn_if_unremovable(const char *op, const char *file, int rc)\n {\n \tint err;\ndiff --git a/wrapper.h b/wrapper.h\nindex c85b1328d163..79a9c1b5077b 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -87,11 +87,6 @@ enum fsync_action {\n */\n int git_fsync(int fd, enum fsync_action action);\n \n-/*\n- * Writes out trace statistics for fsync using the trace2 API.\n- */\n-void trace_git_fsync_stats(void);\n-\n /*\n * Preserves errno, prints a message, but gives no warning for ENOENT.\n * Returns 0 on success, which includes trying to unlink an object that does\n-- \n2.41.0\n\n"},{"id":"479677","messageId":"xmqq5y6e2xl7.fsf@gitster.g","threadId":"60015","inReplyTo":"20230720164823.625815-1-dev+git@drbeat.li","subject":"Re: [PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T19:26:44Z","receivedAt":"2023-07-20T19:26:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Beat Bolli writes:\n\n> As mentioned in the thread starting at [1], trace2 counters should be\n> used to count events instead of ad-hoc static variables.\n>\n> Convert the two fsync static variables to trace2 counters, reducing the\n> coupling between wrapper.c and the trace2 subsystem. Adjust t/t5351 to\n> match the trace2 counter output format.\n>\n> The counters are not per-thread because the ones being replaced also\n> were not.\n>\n> [1] https://lore.kernel.org/git/20230627195251.1973421-2-calvinwan@google.com/\n>\n> Signed-off-by: Beat Bolli \n> ---\n> v2:\n> - Adjust t/t5351\n> - Update commit message\n\nI also spotted this change since v1:\n\n- Rename trace2 counters to use \"-\" (not \"_\") as inter-word separators.\n\nSince I do not seem to be able to find any review comments regarding\nthe variable naming in the v1's thread, let's ask stakeholders.\n\nAre folks involved in the trace2 subsystem (especially Jeff\nHostetler---already CC:ed---who presumably has the most stake in it)\nOK with the naming convention of the multi-word variable? This is\nthe first use of multi-word variable name in tr2_ctr, and thus will\nestablish whatever convention you guys want to use. I do have a\nslight preference of \"writeout-only\" over \"writeout_only\" but that\nis purely from visual appearance. If there is a desire to keep the\nnames literally reusable as identifiers in some languages used to\npostprocess trace output, or something, that might weigh\ndifferently.\n\n> t/t5351-unpack-large-objects.sh | 6 +++---\n> trace2.c | 1 -\n> trace2.h | 4 ++++\n> trace2/tr2_ctr.c | 10 ++++++++++\n> wrapper.c | 19 ++-----------------\n> wrapper.h | 5 -----\n> 6 files changed, 19 insertions(+), 26 deletions(-)\n\nVery nice to see clean-up patch that reduces the amount of code.\nNicely done.\n\nThanks, will queue. If folks do not find issues in a few days,\nlet's merge it to 'next'.\n"},{"id":"479850","messageId":"xmqqo7jzlrdq.fsf@gitster.g","threadId":"60015","inReplyTo":"xmqq5y6e2xl7.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-25T19:31:45Z","receivedAt":"2023-07-25T19:32:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I also spotted this change since v1:\n>\n> - Rename trace2 counters to use \"-\" (not \"_\") as inter-word separators.\n>\n> Since I do not seem to be able to find any review comments regarding\n> the variable naming in the v1's thread, let's ask stakeholders.\n>\n> Are folks involved in the trace2 subsystem (especially Jeff\n> Hostetler---already CC:ed---who presumably has the most stake in it)\n> OK with the naming convention of the multi-word variable? This is\n> the first use of multi-word variable name in tr2_ctr, and thus will\n> establish whatever convention you guys want to use. I do have a\n> slight preference of \"writeout-only\" over \"writeout_only\" but that\n> is purely from visual appearance. If there is a desire to keep the\n> names literally reusable as identifiers in some languages used to\n> postprocess trace output, or something, that might weigh\n> differently.\n\nI heard absolutely nothing since I asked the above question last\nweek, so I'll take the absense of response as absense of interest in\nthe way how names are spelled.\n\nTherefore, let me make a unilateral declaration here ;-) The trace2\ncounters with multi-word names are to be named using \"-\" as their\ninter-word separators. Any patch that adds new counters that do not\nfollow the convention will silently dropped on the floor from now on.\n\nLet's move this patch forward by merging to 'next' soonish.\n\nThanks.\n"},{"id":"479872","messageId":"2f39e481-84d1-097c-ec47-5357dbc36798@drbeat.li","threadId":"60015","inReplyTo":"xmqqo7jzlrdq.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2023-07-25T23:03:54Z","receivedAt":"2023-07-25T23:04:06Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 25.07.23 21:31, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I also spotted this change since v1:\n>>\n>> - Rename trace2 counters to use \"-\" (not \"_\") as inter-word separators.\n>>\n>> Since I do not seem to be able to find any review comments regarding\n>> the variable naming in the v1's thread, let's ask stakeholders.\n>>\n>> Are folks involved in the trace2 subsystem (especially Jeff\n>> Hostetler---already CC:ed---who presumably has the most stake in it)\n>> OK with the naming convention of the multi-word variable? This is\n>> the first use of multi-word variable name in tr2_ctr, and thus will\n>> establish whatever convention you guys want to use. I do have a\n>> slight preference of \"writeout-only\" over \"writeout_only\" but that\n>> is purely from visual appearance. If there is a desire to keep the\n>> names literally reusable as identifiers in some languages used to\n>> postprocess trace output, or something, that might weigh\n>> differently.\n> \n> I heard absolutely nothing since I asked the above question last\n> week, so I'll take the absense of response as absense of interest in\n> the way how names are spelled.\n> \n> Therefore, let me make a unilateral declaration here ;-) The trace2\n> counters with multi-word names are to be named using \"-\" as their\n> inter-word separators. Any patch that adds new counters that do not\n> follow the convention will silently dropped on the floor from now on.\n> \n> Let's move this patch forward by merging to 'next' soonish.\n\nWorks for me :-)\n\nCheers!\n\n"},{"id":"480239","messageId":"60cfab22-821a-4482-f715-12516fc464ef@jeffhostetler.com","threadId":"60015","inReplyTo":"xmqqo7jzlrdq.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-08-07T18:23:58Z","receivedAt":"2023-08-07T18:24:43Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 7/25/23 3:31 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I also spotted this change since v1:\n>>\n>> - Rename trace2 counters to use \"-\" (not \"_\") as inter-word separators.\n>>\n>> Since I do not seem to be able to find any review comments regarding\n>> the variable naming in the v1's thread, let's ask stakeholders.\n>>\n>> Are folks involved in the trace2 subsystem (especially Jeff\n>> Hostetler---already CC:ed---who presumably has the most stake in it)\n>> OK with the naming convention of the multi-word variable? This is\n>> the first use of multi-word variable name in tr2_ctr, and thus will\n>> establish whatever convention you guys want to use. I do have a\n>> slight preference of \"writeout-only\" over \"writeout_only\" but that\n>> is purely from visual appearance. If there is a desire to keep the\n>> names literally reusable as identifiers in some languages used to\n>> postprocess trace output, or something, that might weigh\n>> differently.\n> \n> I heard absolutely nothing since I asked the above question last\n> week, so I'll take the absense of response as absense of interest in\n> the way how names are spelled.\n> \n> Therefore, let me make a unilateral declaration here ;-) The trace2\n> counters with multi-word names are to be named using \"-\" as their\n> inter-word separators. Any patch that adds new counters that do not\n> follow the convention will silently dropped on the floor from now on.\n> \n> Let's move this patch forward by merging to 'next' soonish.\n> \n> Thanks.\n\nSorry I missed before I left for vacation.\n\nMulti-word terms have unfortunately used both \"-\" and \"_\"\nseparators in the past (e.g. builtin/pack-objects.c)\nI don't think it really matters one way or the other.\n\nOriginally, I used \"_\" because there were places where the\npost-processing could more easily extract or query a nested JSON\nor Kusto expression without needing escapes. For example\n`..` rather than something like\n`[\"\"][\"\"]` to avoid having the dash\ninterpreted as subtraction on a local variable).\n\nBut as I and others have added other categories and messages,\nwe've drifted from that usage. And that is fine.\n\nThanks\nJeff\n"},{"id":"480240","messageId":"2a490e5a-2e14-206b-f4ca-73e73e84cf74@jeffhostetler.com","threadId":"60015","inReplyTo":"2f39e481-84d1-097c-ec47-5357dbc36798@drbeat.li","subject":"Re: [PATCH v2 2/2] wrapper: use trace2 counters to collect fsync stats","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-08-07T18:25:09Z","receivedAt":"2023-08-07T18:25:17Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 7/25/23 7:03 PM, Beat Bolli wrote:\n> On 25.07.23 21:31, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> I also spotted this change since v1:\n>>>\n>>> - Rename trace2 counters to use \"-\" (not \"_\") as inter-word separators.\n>>>\n>>> Since I do not seem to be able to find any review comments regarding\n>>> the variable naming in the v1's thread, let's ask stakeholders.\n>>>\n>>> Are folks involved in the trace2 subsystem (especially Jeff\n>>> Hostetler---already CC:ed---who presumably has the most stake in it)\n>>> OK with the naming convention of the multi-word variable?  This is\n>>> the first use of multi-word variable name in tr2_ctr, and thus will\n>>> establish whatever convention you guys want to use.  I do have a\n>>> slight preference of \"writeout-only\" over \"writeout_only\" but that\n>>> is purely from visual appearance.  If there is a desire to keep the\n>>> names literally reusable as identifiers in some languages used to\n>>> postprocess trace output, or something, that might weigh\n>>> differently.\n>>\n>> I heard absolutely nothing since I asked the above question last\n>> week, so I'll take the absense of response as absense of interest in\n>> the way how names are spelled.\n>>\n>> Therefore, let me make a unilateral declaration here ;-)  The trace2\n>> counters with multi-word names are to be named using \"-\" as their\n>> inter-word separators.  Any patch that adds new counters that do not\n>> follow the convention will silently dropped on the floor from now on.\n>>\n>> Let's move this patch forward by merging to 'next' soonish.\n> \n> Works for me :-)\n> \n> Cheers!\n> \n\nAgreed.\n\nThanks\nJeff\n"}]}