You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by ogoodman <gi...@git.apache.org> on 2016/04/20 03:05:19 UTC
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
GitHub user ogoodman opened a pull request:
https://github.com/apache/trafficserver/pull/588
TS-4291 Adds log field "pqnhl" which ignores internal headers.
This should be a very safe fix in that it only adds a new log field and does not change the behaviour of any existing functions. An alternative would be to not add a new log field, but simply change the behaviour of the existing "pqhl" field.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ogoodman/trafficserver TS-4291
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/588.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #588
----
commit f8f7791d2192f8c2168877d3e901bfc6c12203f7
Author: Oliver Goodman <oa...@edge80.com>
Date: 2016-04-20T00:51:57Z
TS-4291 Adds log field "pqnhl" which ignores internal headers.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-218525638
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #588: TS-4291 Adds log field "pqnhl" which ignores inter...
Posted by ogoodman <gi...@git.apache.org>.
Github user ogoodman commented on the issue:
https://github.com/apache/trafficserver/pull/588
Hi Bryan,
Sorry, I probably should have let you know sooner. Please feel free to
close any pull requests of mine.
I have changed jobs and no longer work with Traffic Server. Much as I'd
like to contribute I don't really have the spare time to do so in any
meaningful way.
Regards to yourself and the rest of the team,
-Oliver
On 17/12/16 03:57, Bryan Call wrote:
>
> @ogoodman <https://github.com/ogoodman>
> We talked about this in the github PR meeting and would like to remove
> logging of the @ headers for the current tags.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/trafficserver/pull/588#issuecomment-267640391>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFNB3d4VRFl6gSsPGBft9jOgubsPTUwPks5rIsLngaJpZM4ILQqN>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by ogoodman <gi...@git.apache.org>.
Github user ogoodman commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219261471
Yeah, I'm not sure what happened there. I thought I was rebasing it and that was what somehow caused all those commits to show up. I will see if I can do something about it.
What I need to know though, is whether you really want this new field or would prefer to fix the value of pqhl?
Looking at the documentation made me think that adding a new field would just make an obscure mess and the odds of anyone relying critically on the existing behaviour seem very low.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-218782592
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219186498
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219191872
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #588: TS-4291 Adds log field "pqnhl" which ignores inter...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/588
@ogoodman
We talked about this in the github PR meeting and would like to remove logging of the @ headers for the current tags.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by ogoodman <gi...@git.apache.org>.
Github user ogoodman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60399946
--- Diff: proxy/logging/Log.cc ---
@@ -594,6 +594,11 @@ Log::init_fields()
global_field_list.add(field, false);
ink_hash_table_insert(field_symbol_hash, "pqhl", field);
+ field = new LogField("proxy_req_net_hdr_len", "pqnhl", LogField::sINT, &LogAccess::marshal_proxy_req_net_hdr_len,
+ &LogAccess::unmarshal_int_to_str);
+ global_field_list.add(field, false);
+ ink_hash_table_insert(field_symbol_hash, "pqnhl", field);
+
--- End diff --
Looking at this documentation and re-reading sudheerv's remarks at 8:26 and the eventual wording of the issue lead me to think that adding a new log tag is not the right fix. If you agree I will commit again removing the new field and modifying "pqhl" to match the documentation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219259991
At a minimum, we need a rebase of this before proceeding. This can't really be 100+ commits :).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #588: TS-4291 Adds log field "pqnhl" which ignores inter...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/588
@zwoop @ogoodman I agree that the logging tags should ignore any headers not send on the wire. Let's fix the existing tags rather than make new ones. I would say the current behavior is just a bug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60353113
--- Diff: proxy/hdrs/MIME.cc ---
@@ -1197,6 +1197,27 @@ mime_hdr_length_get(MIMEHdrImpl *mh)
return length;
}
+int
+mime_hdr_net_length_get(MIMEHdrImpl *mh)
--- End diff --
I think it would be better if this function and ``mime_hdr_length_get`` shared an internal implementation, passing a parameter to determine whether to include internal headers.
Since you are studying this code, why is ``length`` initialized to 2? Is it to account for the ``\r\n``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by ogoodman <gi...@git.apache.org>.
Github user ogoodman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60401090
--- Diff: proxy/logging/LogAccessTest.cc ---
@@ -300,6 +300,19 @@ LogAccessTest::marshal_proxy_req_header_len(char *buf)
-------------------------------------------------------------------------*/
int
+LogAccessTest::marshal_proxy_req_net_hdr_len(char *buf)
--- End diff --
Will catch up on IRC: need to clarify what's involved here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60352813
--- Diff: proxy/logging/LogAccessTest.cc ---
@@ -300,6 +300,19 @@ LogAccessTest::marshal_proxy_req_header_len(char *buf)
-------------------------------------------------------------------------*/
int
+LogAccessTest::marshal_proxy_req_net_hdr_len(char *buf)
--- End diff --
ACAICT ``LogAccessTest`` is never actually used. It would be great if you could look into getting this stuff into unit tests (probably in ``traffic_server`` regressions).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-218929156
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #588: TS-4291 Adds log field "pqnhl" which ignore...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall closed the pull request at:
https://github.com/apache/trafficserver/pull/588
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219190148
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-219194920
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/588#issuecomment-220376312
.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60352665
--- Diff: proxy/logging/Log.cc ---
@@ -594,6 +594,11 @@ Log::init_fields()
global_field_list.add(field, false);
ink_hash_table_insert(field_symbol_hash, "pqhl", field);
+ field = new LogField("proxy_req_net_hdr_len", "pqnhl", LogField::sINT, &LogAccess::marshal_proxy_req_net_hdr_len,
+ &LogAccess::unmarshal_int_to_str);
+ global_field_list.add(field, false);
+ ink_hash_table_insert(field_symbol_hash, "pqnhl", field);
+
--- End diff --
Please document the new tag on ``doc/admin-guide/monitoring/logging/log-formats.en.rst``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60352567
--- Diff: proxy/hdrs/HTTP.cc ---
@@ -593,8 +593,8 @@ http_hdr_describe(HdrHeapObjImpl *raw, bool recurse)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
-int
-http_hdr_length_get(HTTPHdrImpl *hdr)
+static int
+http_hdr_base_length_get(HTTPHdrImpl *hdr)
--- End diff --
We should be able to make ``HTTPHdrImpl`` const?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #588: TS-4291 Adds log field "pqnhl" which ignores inter...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/588
So, now that we're on the 7.0.0 release cycle, I wonder if we should have pqhl, cqhl, pshl, and sshl all exclude any @ headers? If not, if we really want this, don't we also want all 4 variants of these tags for consistency? I.e. pqhnl, cqhln, pshln, and sshln ?
@jpeach Wdyt? I'm leaning more towards a 7.0.0 incompatible change and exclude any @ headers from these calculations, but maybe that's just a too expensive change to make (computational expensive).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60352588
--- Diff: proxy/hdrs/HTTP.cc ---
@@ -642,6 +653,19 @@ http_hdr_length_get(HTTPHdrImpl *hdr)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
+int
+http_hdr_net_length_get(HTTPHdrImpl *hdr)
--- End diff --
Please add comments to document how this differs from the other similar header length functions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4291 Adds log field "pqnhl" which i...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/588#discussion_r60352521
--- Diff: proxy/hdrs/HTTP.h ---
@@ -871,6 +873,16 @@ HTTPHdr::length_get()
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
+inline int
+HTTPHdr::net_length_get()
--- End diff --
This should be ``HTTPHdr::net_length_get() const``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---