http: JSON format access logging support #1479

hongzhidao · 2024-10-25T08:26:31Z

Allow format to be an object to generate JSON logs. The object keys
become JSON field names, and values support string, variable, and JS.

Note that when there is no JS in the format values, the object will
be pre-serialized to a JSON template string at configuration phase
for better performance.

Example config:

  {
    "access_log": {
      "path": "/tmp/access.log",
      "format": {
        "remote_addr": "$remote_addr",
        "time_local": "$time_local",
        "request_line": "$request_line",
        "status": "$status",
        "body_bytes_sent": "$body_bytes_sent",
        "header_referer": "$header_referer",
        "header_user_agent": "$header_user_agent"
      }
    }
  }

ac000 · 2024-10-29T16:47:49Z

src/nxt_conf_validation.c

+
+    if (nxt_conf_type(value) != NXT_CONF_STRING) {
+        return nxt_conf_vldt_error(vldt, "In the access log format, the value "
+                                         "must be only string.");


Maybe that could be re-worded slightly to "must be a string."

ac000 · 2024-10-29T16:56:05Z

src/nxt_router_access_log.c

+            next = 0;
+            has_js = 0;


You know what I'm going to say!

These could just be declared (and initialised) here to reduce their scope...

ac000 · 2024-10-29T16:58:56Z

src/nxt_router_access_log.c

+
+                next = 0;
+
+                for (i = 0; i < n; i++) {


Now we're compiling with -std=gnu11, you could take advantage of C99 for loop initialisers... I.e.

for (uint32_t i = 0, i < n; i++) {

ac000 · 2024-10-29T17:02:51Z

src/nxt_router_access_log.c

+    nxt_router_conf_t  *rtcf;
+
+    if (nxt_tstr_is_const(tstr)) {
+        nxt_tstr_str(tstr, &ctx->text);

    } else {


Yep, Mr Reduce Scope again!

} else { nxt_int_t ret; nxt_router_conf_t *rtcf;

ac000 · 2024-10-29T17:04:18Z

src/nxt_router_access_log.c

+        return NXT_ERROR;
+    }
+
+    for (i = 0; i < format->nmembers; i++) {


i could be for loop initialised...

ac000 · 2024-10-29T17:05:04Z

src/nxt_router_access_log.c

+        if (nxt_tstr_is_const(member->tstr)) {
+            nxt_tstr_str(member->tstr, &str);
+
+        } else {


Looks like ret could be declared here...

member could be declared in the for loop...

Good to know, thanks for the suggestions.
I'm fine with both styles. And I'm used to declaring variables at the beginning of functions, so I'd like to keep this way.

Well, it is more than about style, it's good programming practice, which is why I won't stop suggesting it ;)

Note: declaring variables at start of scope is not the same as declaring variables randomly throughout the function, which I certainly wouldn't suggest to do!

I think both ways have pros and cons. Declaring at the start makes all variables visible at a glance and makes reuse easier.

Take this function as an example:

static nxt_int_t nxt_router_access_log_json(nxt_task_t *task, nxt_http_request_t *r, nxt_router_access_log_ctx_t *ctx, nxt_router_access_log_format_t *format) { nxt_uint_t i; nxt_conf_value_t *value; nxt_router_access_log_member_t *member; value = nxt_conf_create_object(r->mem_pool, format->nmembers); if (nxt_slow_path(value == NULL)) { return NXT_ERROR; } for (i = 0; i < format->nmembers; i++) { member = &format->member[i]; ... nxt_conf_set_member_string(value, &member->name, &str, i); } ... return NXT_OK; }

I feel it's common to write like it. If we asked someone to totally follow something like the above "practice", it looks like that:

static nxt_int_t nxt_router_access_log_json(nxt_task_t *task, nxt_http_request_t *r, nxt_router_access_log_ctx_t *ctx, nxt_router_access_log_format_t *format) { nxt_conf_value_t *value; value = nxt_conf_create_object(r->mem_pool, format->nmembers); if (nxt_slow_path(value == NULL)) { return NXT_ERROR; } for (nxt_uint_t i = 0; i < format->nmembers; i++) { nxt_router_access_log_member_t *member; member = &format->member[i]; ... nxt_conf_set_member_string(value, &member->name, &str, i); } ... return NXT_OK; }

From the point of code style, both ways look fine.
It doesn't seem to make a difference in terms of whether it's more buggy or not.

I'm just using this function as an example, because it's not big, and I'm not sure if it's going to be different if it's really big

I discussed it based on the specific code which I listed above. Please do not expand the scope and apply one "principle" to all situations.

I am talking generally (not specific examples)...

Reducing variable scope is just good programming practice... ipso facto...

Take nxt_router_access_log_json() as an example, i and member are only used in the for sentence, I can't see benefits to force their declaration within it.
I would be confused if it can introduce issues in this way, I'm only talking about the function since we are reviewing it.

Take nxt_router_access_log_json() as an example, i and member are only used in the for sentence,

Yes, that's exactly the point! They are only used in the for loop so that is where they should be scoped.

I can't see benefits to force their declaration within it.

I already gave an example of a benefit...

I would be confused if it can introduce issues in this way, I'm only talking about the function since we are reviewing it.

Never underestimate the propensity for introducing bugs.

I wonder if a function is as small as array_sum(), should it follow the practice?

Certainly no harm... and it's more concise to read...

ac000

Assuming this doesn't change wildly...

This is a preparatory refactoring for upcoming JSON format support in access log. No functional changes.

This is a preparatory refactoring for upcoming JSON format support in access log. We will extend format option to access object for JSON support. No functional changes.

Allow format to be an object to generate JSON logs. The object keys become JSON field names, and values support string, variable, and JS. Note that when there is no JS in the format values, the object will be pre-serialized to a JSON template string at configuration phase for better performance. Example config: { "access_log": { "path": "/tmp/access.log", "format": { "remote_addr": "$remote_addr", "time_local": "$time_local", "request_line": "$request_line", "status": "$status", "body_bytes_sent": "$body_bytes_sent", "header_referer": "$header_referer", "header_user_agent": "$header_user_agent" } } }

hongzhidao marked this pull request as draft October 25, 2024 08:27

hongzhidao force-pushed the json-access-log branch from 996b9c6 to 7339197 Compare October 29, 2024 05:10

hongzhidao marked this pull request as ready for review October 29, 2024 05:19

hongzhidao force-pushed the json-access-log branch from 7339197 to d498421 Compare October 29, 2024 07:35

ac000 reviewed Oct 29, 2024

View reviewed changes

hongzhidao force-pushed the json-access-log branch 2 times, most recently from 8e2149a to d4fa165 Compare November 7, 2024 05:12

ac000 approved these changes Nov 13, 2024

View reviewed changes

hongzhidao added 5 commits November 14, 2024 09:43

Make nxt_tstr_is_js() macro public in header

2d4624f

This is a preparatory refactoring for upcoming JSON format support in access log. No functional changes.

http: Refactor format field in nxt_router_access_log_conf_t

a92d814

This is a preparatory refactoring for upcoming JSON format support in access log. No functional changes.

http: Introduce nxt_router_access_log_format_t structure

a760e24

This is a preparatory refactoring for upcoming JSON format support in access log. We will extend format option to access object for JSON support. No functional changes.

tests: Add tests for JSON format access log

a355179

hongzhidao force-pushed the json-access-log branch from d4fa165 to a355179 Compare November 14, 2024 01:44

hongzhidao merged commit a355179 into nginx:master Nov 14, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: JSON format access logging support #1479

http: JSON format access logging support #1479

hongzhidao commented Oct 25, 2024 •

edited

Loading

ac000 Oct 29, 2024

ac000 Oct 29, 2024

ac000 Oct 29, 2024

ac000 Oct 29, 2024

ac000 Oct 29, 2024

ac000 Oct 29, 2024

ac000 Oct 29, 2024

hongzhidao Oct 30, 2024

ac000 Oct 30, 2024

hongzhidao Oct 30, 2024 •

edited

Loading

ac000 Oct 31, 2024

hongzhidao Oct 31, 2024

ac000 Oct 31, 2024

hongzhidao Oct 31, 2024

ac000 Oct 31, 2024

ac000 left a comment

http: JSON format access logging support #1479

http: JSON format access logging support #1479

Conversation

hongzhidao commented Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongzhidao Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ac000 left a comment

Choose a reason for hiding this comment

hongzhidao commented Oct 25, 2024 •

edited

Loading

hongzhidao Oct 30, 2024 •

edited

Loading