You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2015/11/24 22:11:52 UTC
[GitHub] trafficserver pull request: TS-3072: Debug logging for single conn...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/350
TS-3072: Debug logging for single connection.
Proposed changes to enable per-client-ip specification for debug messages. A version of this code has been running in our production for the past month. We are able to capture debug messages from the start of the TCP connection.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-3072
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/350.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 #350
----
----
---
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-3072: Debug logging for single conn...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161205132
To quote our own docs (under Admin Guide/Plugins):
"""
One of the key features of Apache Traffic Server™ is its modularity. Features that aren’t needed in the core simply aren’t there. This helps to provide an additional guarantee that our core can remain fast by concentrating on the things that we always provide: caching and proxying.
"""
I'd add that keeping things out of the core also makes it easier to understand, verify and debug.
The way I see it - Logic on when to enable debugging on a transaction/session can be arbitrarily complex. I'm sure everyone on this thread can see the use of enabling debugs based on things like cipher suites, TLS protocol version, destination IPs. Even when relying only on source IPs, you might want to enable for only one in ten connections to cut the logging volume.
Our (badly named :-) ) header_rewrite plugin can easily be adjusted to support such logic.
(Personally - my trigger to enable debugging is often determined in an external process that does policy matching)
@djcarlin - This being implemented in a plugin doesn't mean you'd have to restart ATS to use it -- The plugin would be loaded by default and you could enable/disable debugging by modifying it's configuration.
(You could even talk to it directly via traffic_line using @SolidWallOfCode latest work on TS-4032)
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161034092
Originally, I just had an int pointer for the flags. Then we were doing experiments with multiple flags, and it seemed like having a separate class with bit names was more readable.
From a runtime implementation perspective, I think it is all about the same.
---
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-3072: Debug logging for single conn...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159416923
See the discussion here: http://wilderness.apache.org/channels/?f=traffic-server/2015-11-17#1447797537
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-166934284
Created new PR #398
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/350
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159436716
W.r.t. UnixNetVConnection pulling in Diags, that is kind of disturbing and can be cleaned up I think. The network area can read in the client_ip config and do the test. If we decide to keep the config-based control, I'll fix that up.
Of course we are dragging in the global diag everywhere we are calling the Debug() macros. I removed the extra reference to Diags.h, and everything still built.
---
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-3072: Debug logging for single conn...
Posted by djcarlin <gi...@git.apache.org>.
Github user djcarlin commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-160751958
I like it being built into core - we can enable per-ip debugging without restarting ATS since the setting is overridable. This way we don't end up clearing the condition we were trying to debug.
Its also a lot easier to use this way; I can enable via traffic_line setting versus configuring a plugin, draining traffic from the host, restarting ATS to load the plugin, etc.
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159433112
Ah, forgot to add the new file. Will get them in.
---
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-3072: Debug logging for single conn...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159412153
I had an IRC discussion with sudheerv about this change during the summit -- I strongly feel that if this can be implemented via a plugin (and I think it the SSN_START happens early enough), it should be.
The bottom line of the IRC conversation was that he would bring this up for a vote.....
---
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-3072: Debug logging for single conn...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161030286
Yeh I mean the latter. All the code in ```ContFlags``` boils down to getspecific + setspecific, the rest is really not necessary. A separate file with a class that is used only once (and it not really re-usable) spreads the code around in a way that makes it harder to follow.
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159413805
From my understanding of the code the SSN_START hooks happen after the SSL connections have been negotiated. So you would miss debug messages there.
I would assume that you would miss some startup messages from HTTP2/SPDY too particularly since our SSN/TXN differentiation is mixed up after the addition of multiplexing protocols like HTTP2 and SPDY.
---
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-3072: Debug logging for single conn...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159416537
Well -- For SSL you could use the SNI callback, though I agree that adding a 'new connection' hookpoint would be nicer.
To clarify -- I'm all for supporting the continuation based override (Note that it partially obsoletes the per HttpSM/ClientSession override that's already present). I'm just saying that I think the logic on when to turn it on should be in plugin space....
---
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-3072: Debug logging for single conn...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161370254
@ushachar I added support for debugging in header_rewrite and it has the ability to do client ip matching. As @SolidWallOfCode pointed out there may be some use cases where you might want to debug the TLS handshake and would require an earlier hook.
I have felt this feature should be part of a plugin and it is in two already (header_rewrite and XDebug). If debugging required at an earlier stage add a hook and modify the plugins. If on the fly debugging is required then modify the existing plugins to reload their configurations. Added a third way to turn on debugging will only make things more confusing.
---
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-3072: Debug logging for single conn...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159640428
We can use the PR for voting? If an email is needed, perhaps, @shinrich might send a vote request, given that she did the current patch.
In any case, my vote is +1 on having this feature in the core :)
---
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-3072: Debug logging for single conn...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161026974
James - a `uintptr_t` instead of `ContFlags` would require the assumption a continuation always runs on the same thread, which is not the case. `ContFlags` ensures that the continuation gets the thread based flags regardless of the thread on which it executes. On the other hand if you mean doing the thread specific stuff in `Continuation` I would think it's clearly better to isolate that stuff in `ContFlags`.
---
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-3072: Debug logging for single conn...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159414518
Yes. You would need a new hook that gets called after the TCP connection (immediately after accept() returns the new socket). Also, how would the plugin enable debug messages for just that user agent connection?
---
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-3072: Debug logging for single conn...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159698637
I'll send a note around next Monday when folks are back from Thanksgiving.
---
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-3072: Debug logging for single conn...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161358854
Uri - it is not the case that header_rewrite can support the required use case. There is no hook that is sufficiently early to enable pre-SSL accept debugging and therefore no plugin can actually enable it that early. I understand your point and in general agree with it, but the fact remains it is not possible to do it with the current set of hooks. If you want to propose adding a hook to make it possible (which would have to be immediately after the TCP handshake) that would be good. That is outside the scope of this PR.
---
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-3072: Debug logging for single conn...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161024984
The whole ```ContFlags``` class seems unnecessary. If you really need per-thread flags, then why not just keep a thread-specify ```uintptr_t``` in the ```Continuation```?
---
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-3072: Debug logging for single conn...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-159418105
```ContFlags.{cc,h}``` appear to be missing. I'm with @ushachar on this; I think the capability is useful but I'm concerned about having the policy in the core.
We need to be really careful about dependencies here. For example, I don't know that ```P_UnixNetVConnection.h``` should drag in ```Diags.h```.
---
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-3072: Debug logging for single conn...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-161377617
Uri - I agree with your point in general as well, but, I think it's arguable whether to treat this as a *feature* that isn't needed in the core. I can say, with certainty, that a majority (if not all) of production deployments would be benefitted from this capability. Having this in the core (even in its simplest form) already provides a great benefit to them, in terms of simplicity in configuration and use. Having to deploy a plugin and maintain it's configuration separately only makes it more complicated and less easy to use for general operations.
Fwiw, we also have a number of similar features (which could have all been done using plugins too) in the core that are used extensively by operations and plugin developers alike (e.g @ 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-3072: Debug logging for single conn...
Posted by mlibbey <gi...@git.apache.org>.
Github user mlibbey commented on the pull request:
https://github.com/apache/trafficserver/pull/350#issuecomment-160755930
We have health check and other "random" traffic to machines that make reading debug logs really painful. As a result, we are constantly wishing for a way to turn debugging on for a single remap rule.
---
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.
---