From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id CA40B1F5AE; Sun, 2 Aug 2020 18:55:57 +0000 (UTC) Date: Sun, 2 Aug 2020 18:55:57 +0000 From: Eric Wong To: Josh Natanson Cc: clogger-public@yhbt.net Subject: Re: [PATCH] Added optional POWER argutment to $response_time. Message-ID: <20200802185557.GA21310@dcvr> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: List-Id: Josh Natanson wrote: > Subject: Re: [PATCH] Added optional POWER argutment to $response_time. Hi Josh, spelling: "argument" Btw, I'm no math expert, so I'm not sure if "power" is even the correct term, here. Is "order-of-magnitude" more correct? (or just "ORDER"). Also, no need for "." in commit titles (or email subjects). > This argument allows for conversion of response_time to microsecond > or nanosecond, up to a limit of 9. Defaults to 0 so backwards compatible. > --- > README | 10 ++++++---- > ext/clogger_ext/clogger.c | 28 ++++++++++++++++++++++++++++ > lib/clogger.rb | 11 +++++++++-- > lib/clogger/pure.rb | 1 + > test/test_clogger.rb | 25 +++++++++++++++++++++++-- > 5 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/README b/README > index 1d1d537..7c22fd5 100644 > --- a/README > +++ b/README > @@ -69,10 +69,12 @@ that receives a "<<" method: > * $request_uri - the URI requested ($path_info?$query_string) > * $request - the first line of the HTTP request > ($request_method $request_uri $http_version) > -* $request_time, $request_time{PRECISION} - time taken for request > - (including response body iteration). PRECISION defaults to 3 > - (milliseconds) if not specified but may be specified anywhere from > - 0(seconds) to 6(microseconds). > +* $request_time, $request_time{PRECISION}, $request_time{POWER, PRECISION} - There's a space between "POWER," and "PRECISION" in the documentation. However, the code below doesn't accept a space (and I prefer it not). > + time taken for request (including response body iteration). PRECISION > + defaults to 3 (milliseconds) if not specified but may be specified > + anywhere from 0(seconds) to 6(microseconds). POWER will raise the time to > + the provided power of 10, useful for converting to micro- or nanoseconds. > + POWER defaults to 0 if not specified but may be specified any from 0 to 10 > * $time_iso8601 - current local time in ISO 8601 format, > e.g. "1970-01-01T00:00:00+00:00" > * $time_local - current local time in Apache log format, > diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c > index fdc23e3..a1f3bc4 100644 > --- a/ext/clogger_ext/clogger.c > +++ b/ext/clogger_ext/clogger.c > @@ -374,12 +374,40 @@ static void append_ts(struct clogger *c, VALUE > op, struct timespec *ts) > rb_str_buf_cat(c->log_buf, buf, nr); > } > > +#define NANO_PER_SEC (1000 * 1000 * 1000) > static void append_request_time_fmt(struct clogger *c, VALUE op) > { > struct timespec now; > + unsigned long ipow = NUM2ULONG(rb_ary_entry(op, 3)); While the space-indented Ruby parts came out fine, the .c code uses hard tabs and they weren't preserved in this patch. Based on your headers, it seems like you're using the gmail web GUI instead of a mail client; and that doesn't preserve hard tabs in inline messages. Quoting Documentation/process/email-clients.rst from linux.git(*): > Gmail (Web GUI) > *************** > > Does not work for sending patches. > > Gmail web client converts tabs to spaces automatically. > > At the same time it wraps lines every 78 chars with CRLF style line breaks > although tab2space problem can be solved with external editor. Again, for one-off patches, "git format-patch"-generated attachments are probably fine if you don't want to configure "git send-email". Btw, I realize some of this can seem nitpicky and strange to you; but I'm really trying to get the world away from monopolies, centralized tools; along with preparing you and others to contribute to other projects with a decentralized, asynchronous workflow. (*) https://yhbt.net/mirrors/linux.git/plain/Documentation/process/email-clients.rst > diff --git a/lib/clogger.rb b/lib/clogger.rb > index be1bdce..7ce2b24 100644 > --- a/lib/clogger.rb > +++ b/lib/clogger.rb > @@ -51,7 +51,7 @@ private > > SCAN = /([^$]*)(\$+(?:env\{\w+(?:\.[\w\.]+)?\}| > e\{[^\}]+\}| > - (?:request_)?time\{\d+\}| > + (?:request_)?time\{\d+(?:,\d+)?\}| > time_(?:utc|local)\{[^\}]+\}| > \w*))?([^$]*)/x No spaces here between the args, which confirms my comments about the README change. The rest of the patch looked fine. Can you take another shot at resending with my comments addressed? No rush, though. Thanks again.