Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using vars multiple times in access_log.format does not work correctly with njs #1169

Closed
valtzu opened this issue Mar 2, 2024 · 3 comments · Fixed by #1176
Closed

Using vars multiple times in access_log.format does not work correctly with njs #1169

valtzu opened this issue Mar 2, 2024 · 3 comments · Fixed by #1176
Assignees
Labels
T-Defect Bugs, crashes, hangs, and other problems

Comments

@valtzu
Copy link

valtzu commented Mar 2, 2024

Related to recently added feature #1024.


When using NJS-based access log format (with Docker unit 1.32.0), the value of first vars is used for all the rest vars – see the example below.

Config

{
  "access_log": {
    "path": "/dev/stderr",
    "format": "`${JSON.stringify({bodyLength:vars.body_bytes_sent,status:vars.status})}\n`"
  }
}

or

  "format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"

Log output:

unit-1  | {"bodyLength":"82733","status":"82733"}

Whereas with non-js version "format": "bodyLength=$body_bytes_sent status=$status" it works fine:

unit-1  | bodyLength=82733 status=200
@valtzu
Copy link
Author

valtzu commented Mar 2, 2024

Actually this behavior seems to be random – sometimes it works, sometimes it doesn't 🤔

unit-1  | {"bodyLength":"82733","status":"82733"}
unit-1  | {"bodyLength":"82735","status":"82735"}
unit-1  | {"bodyLength":"82735","status":"200"}
unit-1  | {"bodyLength":"82735","status":"82735"}
unit-1  | {"bodyLength":"82735","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}
unit-1  | {"bodyLength":"82734","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}
unit-1  | {"bodyLength":"82736","status":"200"}

@tippexs
Copy link
Contributor

tippexs commented Mar 3, 2024

Hi @valtzu Thanks for reaching out! This looks indeed not correct and I was able to reproduce the issue with the latest version of our Docker-Image and the format mentioned above.

We will have a look on this and share our progress here! Thanks again for reaching out!

@tippexs tippexs added the T-Defect Bugs, crashes, hangs, and other problems label Mar 3, 2024
@hongzhidao hongzhidao self-assigned this Mar 5, 2024
@hongzhidao
Copy link
Contributor

Hi @valtzu,
Thanks for the reporting. Will fix it soon with the patch.

diff --git a/src/nxt_var.c b/src/nxt_var.c
index 2600371b..57110f66 100644
--- a/src/nxt_var.c
+++ b/src/nxt_var.c
@@ -147,7 +147,7 @@ nxt_var_ref_get(nxt_tstr_state_t *state, nxt_str_t *name, nxt_mp_t *mp)

     if (decl != NULL) {
         ref->handler = decl->handler;
-        ref->cacheable = decl->cacheable;
+        ref->cacheable = (mp == state->pool) ? decl->cacheable : 0;

         goto done;
     }

hongzhidao added a commit to hongzhidao/unit that referenced this issue Mar 6, 2024
The variables accessed with JS template literal should be not cacheable.
Since it is parsed by njs engine, Unit can't create indexes on these
variables for caching purpose. For example:

   {
       "format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"
   }

The variables like the above are not cacheable.

Closes: nginx#1169
hongzhidao added a commit to hongzhidao/unit that referenced this issue Mar 6, 2024
The variables accessed with JS template literal should be not cacheable.
Since it is parsed by njs engine, Unit can't create indexes on these
variables for caching purpose. For example:

   {
       "format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"
   }

The variables like the above are not cacheable.

Closes: nginx#1169
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Mar 11, 2024
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Mar 11, 2024
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Mar 11, 2024
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Mar 12, 2024
andrey-zelenkov added a commit that referenced this issue Mar 12, 2024
andrey-zelenkov pushed a commit that referenced this issue Mar 15, 2024
The variables accessed with JS template literal should not be cacheable.
Since it is parsed by njs engine, Unit can't create indexes on these
variables for caching purpose. For example:

   {
       "format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"
   }

The variables like the above are not cacheable.

Closes: #1169
andrey-zelenkov added a commit that referenced this issue Mar 15, 2024
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
The variables accessed with JS template literal should not be cacheable.
Since it is parsed by njs engine, Unit can't create indexes on these
variables for caching purpose. For example:

   {
       "format": "`{bodyLength:\"${vars.body_bytes_sent}\",status:\"${vars.status}\"}\n`"
   }

The variables like the above are not cacheable.

Closes: nginx#1169
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, and other problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants