Modify log to add ngx.var.uri in request object #2445

shiprabehera · 2017-04-24T10:27:41Z

Summary

ngx.var.request_uri is the original uri and is not updated if the uri is changed using ngx.set_uri(newUri). Instead, ngx.var.uri reflects the new uri. Added a new field to reflect the modified uri.

Full changelog

Use the same name as the Nginx variables in request object of logs
Rename request_uri to request_url
New unit test

Issues resolved

Fix #2441

p0pr0ck5 · 2017-04-24T16:22:08Z

request_uri contains query arguments, uri does not. If the only difference here is a post-normalized representation, I think we need to append the query string. Thoughts?

thibaultcha

Hi,

Thanks for the patch!

thibaultcha · 2017-04-24T17:58:50Z

kong/plugins/log-serializers/basic.lua

@@ -16,6 +16,7 @@ function _M.serialize(ngx)
  return {
    request = {
      uri = ngx.var.request_uri,
+      new_uri = ngx.var.uri,


This definitely warrants a more careful approach or at least some discussion. We must make it obvious what the difference between those two values is. Two options:

use the same name as the Nginx variables (breaking change), and document them as being such.

keep uri, but rename new_uri to upstream_uri. Since we diverge from the Nginx variables, we would need to make them look similar, and would have to include the query string as @p0pr0ck5 suggested. However, there will still exist cases where uri will be normalized (path normalization, percent-decoded...), and upstream_uri will not. There will also be cases (most of the time probably), where both uri and upstream_uri will be identical, which could be confusing and/or wasteful.

I tend to lean towards 1., and open this against next, as a documented breaking change.

This will also need tests btw :)

@pablofmorales , correct. But the value of $uri can change - for example during redirects, which is not captured by $request_uri
@thibaultcha I agree that for most cases, both ngx.var.request_uri and ngx.var.uri would be same. Hence, renaming them could lead to confusion. In fact, this would only be useful if there is some redirect or uri modification happening and I want to log those as well. Let's use the same name as option 1 suggests. However, then we would have to modify the name of the field request_uri too. Maybe request_url ? :)

request_uri = ngx.var.request_uri, uri = ngx.var.uri, request_url = ngx.var.scheme.."://"..ngx.var.host..":"..ngx.var.server_port..ngx.var.request_uri

I will add test cases, thanks.

p0pr0ck5 · 2017-05-31T16:36:38Z

@shiprabehera any update on the changes requested above?

p0pr0ck5 · 2017-06-13T15:43:08Z

Thanks for the update @shiprabehera! There is a merge conflict in the serializer though. Can we resolve this?

p0pr0ck5 · 2017-06-16T20:54:43Z

kong/plugins/log-serializers/basic.lua

-      request_uri = ngx.var.scheme .. "://" .. ngx.var.host .. ":" .. ngx.var.server_port .. ngx.var.request_uri,
+      request_uri = ngx.var.request_uri,
+      uri = ngx.var.uri,
+      request_url = ngx.var.scheme.."://"..ngx.var.host..":"..ngx.var.server_port..ngx.var.request_uri,


so, this renames request_uri -> request_url, and makes request_uri a new value (the $request_uri) variable? this feels odd, and very much a breaking change. since we're introducing a new value request_url, why not just give it the new variable assignment?

Yes, this is a breaking change indeed. The whole point is to name the variables same as the Nginx variables to avoid confusion. Also, if we are mentioning parameters like scheme and host, it makes more sense to rename it to request_url, i.e url which is more specific as opposed to uri, no?

@shiprabehera thanks. Since this is a breaking change, we will need to point this change to the next branch (you can simply edit this PR), and will need to be rebased accordingly. We may also want to take some naming changes in 0010bce into account as well (not sure they're entirely relevant, but it's worth being aware of).

shiprabehera · 2017-07-06T13:30:42Z

@p0pr0ck5 , thanks for mentioning that.
0010bce has core router changes and does not refer to variables from the log file I have updated. This patch makes log variable names same as Nginx, does not change any core functionality.
Also, few integration test suites seem to fail. Could this be related?
Thanks.

p0pr0ck5 · 2017-07-18T20:29:54Z

Hi @shiprabehera, sorry for the delay on this end. I will look over this again soon and let you know!

p0pr0ck5 · 2017-08-01T05:21:37Z

@thibaultcha do you still have pending requested changes here? I still need to review this :) Just catching up on any blockers.

thibaultcha · 2017-08-01T05:45:27Z

@p0pr0ck5 Nothing outstanding! This is looking good to me, at a glance. Although, if we are about to break to introduce more meaningful names to those properties, I'm thinking: "why not renaming uri to upstream_uri...

lgtm

p0pr0ck5 · 2017-08-07T16:20:18Z

Thanks thibaultcha!

why not renaming uri to upstream_uri

@shiprabehera any thoughts on this comment?

shiprabehera · 2017-08-08T01:15:32Z

@p0pr0ck5 @thibaultcha
Sounds good! As long as we have clear distinction between them, no problem.

thibaultcha

Everything looks good now except for this value to update since 0.11.0 :)

thibaultcha · 2017-08-19T02:35:50Z

kong/plugins/log-serializers/basic.lua

-      uri = ngx.var.request_uri,
-      request_uri = ngx.var.scheme .. "://" .. ngx.var.host .. ":" .. ngx.var.server_port .. ngx.var.request_uri,
+      request_uri = ngx.var.request_uri,
+      upstream_uri = ngx.var.uri,


Since 0.11.0, the upstream URI is determined via ngx.var.upstream_uri, and not set via ngx.req.set_uri() anymore, which means that this value need to be updated from ngx.var.uri to ngx.var.upstream_uri. Could you take care of that?

shiprabehera · 2017-08-21T11:54:20Z

@thibaultcha
Fixed it.

p0pr0ck5 · 2017-08-23T16:58:22Z

This LGTM. Will wait for @thibaultcha to dismiss the review, then I think we're good to go here.

thibaultcha · 2017-08-25T20:11:14Z

Looking good to me! Good to highlight this will be our first breaking change introduced after the 0.11.0 release - which means once this gets merged to next, it will be the first non-backwards compatible change compared to master!

thibaultcha · 2017-08-25T20:12:21Z

(It looks like this patch could use some rebasing though - @shiprabehera will you take care of it to release a bit of the burden from @p0pr0ck5's shoulders? 😄)

thibaultcha · 2017-08-28T22:31:17Z

@shiprabehera Thanks for giving it a try, but could we have only 1 commit here, like:

feat(log) improve uri fields in basic log serializer

* add `request.upstream_uri`
* rename `request.uri` to `request.request_uri`
* rename `request.request_uri` to `request.request_url`

From #2445

And no merge commit at all?

Thanks!

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From Kong#2445

shiprabehera · 2017-08-29T03:10:26Z

@thibaultcha
Done. Thanks!

thibaultcha · 2017-08-29T03:11:46Z

@shiprabehera Great, thank you!

thibaultcha · 2017-11-01T23:09:40Z

Merged! Thank you @shiprabehera - and sorry for the delay on our side. As per our breaking change policy, this should be scheduled for release in our next major version.

shiprabehera · 2017-11-02T08:05:29Z

Thank you @thibaultcha

Reflecting back on #2445, the chosen names feel very redundant when accessed from a queryable interface. The same way, `upstream_uri` seems to have been wrongly added under the `request` scope, where it doesn't belong. This solution has the benefit of being less breaking as well. (Only one field gets renamed).

Reflecting back on #2445, the chosen names feel very redundant when accessed from a queryable interface. The same way, `upstream_uri` seems to have been wrongly added under the `request` scope, where it doesn't belong. This solution has the benefit of being less breaking as well. (Only one field gets renamed). From #3098

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From #2445

Reflecting back on #2445, the chosen names feel very redundant when accessed from a queryable interface. The same way, `upstream_uri` seems to have been wrongly added under the `request` scope, where it doesn't belong. This solution has the benefit of being less breaking as well. (Only one field gets renamed). From #3098

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From #2445

Reflecting back on #2445, the chosen names feel very redundant when accessed from a queryable interface. The same way, `upstream_uri` seems to have been wrongly added under the `request` scope, where it doesn't belong. This solution has the benefit of being less breaking as well. (Only one field gets renamed). From #3098

thibaultcha previously requested changes Apr 24, 2017

View reviewed changes

thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Apr 24, 2017

p0pr0ck5 self-assigned this May 30, 2017

p0pr0ck5 reviewed Jun 16, 2017

View reviewed changes

shiprabehera changed the base branch from master to next July 6, 2017 12:29

thibaultcha mentioned this pull request Jul 7, 2017

TCP logs do not show updated request.uri #2441

Closed

p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jul 18, 2017

thibaultcha requested changes Aug 19, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Aug 19, 2017

p0pr0ck5 added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Aug 23, 2017

thibaultcha approved these changes Aug 25, 2017

View reviewed changes

thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Aug 28, 2017

thibaultcha removed the pr/status/please review label Aug 28, 2017

shiprabehera force-pushed the master branch from 344aec9 to 9810df5 Compare August 29, 2017 02:18

feat(log) improve uri fields in basic log serializer

4adb1d0

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From Kong#2445

shiprabehera force-pushed the master branch from 9810df5 to 4adb1d0 Compare August 29, 2017 03:00

thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Aug 29, 2017

thibaultcha merged commit c9d6187 into Kong:next Nov 1, 2017

thibaultcha mentioned this pull request Dec 15, 2017

hotfix(log-serializer) avoid redundant request.request_* properties #3098

Merged

thibaultcha pushed a commit that referenced this pull request Jan 16, 2018

feat(log) improve uri fields in basic log serializer

0989bac

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From #2445

thibaultcha pushed a commit that referenced this pull request Jan 19, 2018

feat(log) improve uri fields in basic log serializer

d24b76f

* add `request.upstream_uri` * rename `request.uri` to `request.request_uri` * rename `request.request_uri` to `request.request_url` From #2445

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify log to add ngx.var.uri in request object #2445

Modify log to add ngx.var.uri in request object #2445

shiprabehera commented Apr 24, 2017 •

edited

Loading

p0pr0ck5 commented Apr 24, 2017

thibaultcha left a comment

thibaultcha Apr 24, 2017

shiprabehera Apr 25, 2017

p0pr0ck5 commented May 31, 2017

p0pr0ck5 commented Jun 13, 2017

p0pr0ck5 Jun 16, 2017

shiprabehera Jun 20, 2017

p0pr0ck5 Jun 21, 2017

shiprabehera commented Jul 6, 2017 •

edited

Loading

p0pr0ck5 commented Jul 18, 2017

p0pr0ck5 commented Aug 1, 2017

thibaultcha commented Aug 1, 2017

p0pr0ck5 commented Aug 7, 2017

shiprabehera commented Aug 8, 2017 •

edited

Loading

thibaultcha left a comment

thibaultcha Aug 19, 2017

shiprabehera commented Aug 21, 2017

p0pr0ck5 commented Aug 23, 2017

thibaultcha commented Aug 25, 2017

thibaultcha commented Aug 25, 2017

thibaultcha commented Aug 28, 2017

shiprabehera commented Aug 29, 2017

thibaultcha commented Aug 29, 2017

thibaultcha commented Nov 1, 2017

shiprabehera commented Nov 2, 2017

Modify log to add ngx.var.uri in request object #2445

Modify log to add ngx.var.uri in request object #2445

Conversation

shiprabehera commented Apr 24, 2017 • edited Loading

Summary

Full changelog

Issues resolved

p0pr0ck5 commented Apr 24, 2017

thibaultcha left a comment

Choose a reason for hiding this comment

thibaultcha Apr 24, 2017

Choose a reason for hiding this comment

shiprabehera Apr 25, 2017

Choose a reason for hiding this comment

p0pr0ck5 commented May 31, 2017

p0pr0ck5 commented Jun 13, 2017

p0pr0ck5 Jun 16, 2017

Choose a reason for hiding this comment

shiprabehera Jun 20, 2017

Choose a reason for hiding this comment

p0pr0ck5 Jun 21, 2017

Choose a reason for hiding this comment

shiprabehera commented Jul 6, 2017 • edited Loading

p0pr0ck5 commented Jul 18, 2017

p0pr0ck5 commented Aug 1, 2017

thibaultcha commented Aug 1, 2017

p0pr0ck5 commented Aug 7, 2017

shiprabehera commented Aug 8, 2017 • edited Loading

thibaultcha left a comment

Choose a reason for hiding this comment

thibaultcha Aug 19, 2017

Choose a reason for hiding this comment

shiprabehera commented Aug 21, 2017

p0pr0ck5 commented Aug 23, 2017

thibaultcha commented Aug 25, 2017

thibaultcha commented Aug 25, 2017

thibaultcha commented Aug 28, 2017

shiprabehera commented Aug 29, 2017

thibaultcha commented Aug 29, 2017

thibaultcha commented Nov 1, 2017

shiprabehera commented Nov 2, 2017

shiprabehera commented Apr 24, 2017 •

edited

Loading

shiprabehera commented Jul 6, 2017 •

edited

Loading

shiprabehera commented Aug 8, 2017 •

edited

Loading