Re: [PATCH] Added optional POWER argutment to $response_time.

[Eric Wong <e@yhbt.net>]

Josh Natanson <josh@natanson.net> 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.