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

TS-4291 Adds log field "pqnhl" which ignores internal headers. #588

Closed
wants to merge 3 commits into from

Conversation

ogoodman
Copy link
Contributor

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.

@@ -871,6 +873,16 @@ HTTPHdr::length_get()
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/

inline int
HTTPHdr::net_length_get()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be HTTPHdr::net_length_get() const.

@zwoop zwoop added the Logging label May 12, 2016
@zwoop
Copy link
Contributor

zwoop commented May 15, 2016

At a minimum, we need a rebase of this before proceeding. This can't really be 100+ commits :).

@zwoop zwoop added this to the 7.0.0 milestone May 15, 2016
@ogoodman
Copy link
Contributor Author

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.

@zwoop zwoop self-assigned this Jun 27, 2016
@zwoop
Copy link
Contributor

zwoop commented Jul 20, 2016

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).

@jpeach
Copy link
Contributor

jpeach commented Aug 21, 2016

@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.

@zwoop zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016
@zwoop zwoop removed their assignment Dec 16, 2016
@bryancall
Copy link
Contributor

@ogoodman
We talked about this in the github PR meeting and would like to remove logging of the @ headers for the current tags.

@ogoodman
Copy link
Contributor Author

ogoodman commented Dec 16, 2016 via email

@zwoop zwoop modified the milestones: 7.1.0, 7.2.0 Jan 8, 2017
@bryancall bryancall closed this Jan 24, 2017
@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@zwoop zwoop removed this from the 7.2.0 milestone Apr 25, 2017
@zwoop zwoop modified the milestone: 8.0.0 May 4, 2017
shinrich pushed a commit to shinrich/trafficserver that referenced this pull request Jan 9, 2018
Added skip condition on http2 test for lack of curl http2
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants