-
Notifications
You must be signed in to change notification settings - Fork 73
feat(handler): log tunnel close with destination and duration #238
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
Open
m11y
wants to merge
2
commits into
SenseUnit:master
Choose a base branch
from
m11y:feat/tunnel-close-duration-log
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be useful to put it at debug log level, but having it on default one would be too much of output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I understand the concern about default log volume.
My thinking behind keeping this at Info was that it is symmetric with the existing CONNECT request log: one line when the tunnel is opened, and one corresponding line when it is closed. This close log is emitted once per tunnel, not per copy/read/write operation, so it completes the lifecycle record rather than adding continuous noise.
The reason I would prefer Info is that the close side is usually the important part when diagnosing production EOF/timeout issues. If it is only available at Debug, users need to enable Debug before reproducing the problem, and the open/close pair ends up split across different log levels.
That said, I understand the desire to keep default output minimal. Would you be open to keeping it at Info since it mirrors the existing CONNECT log, or do you still strongly prefer Debug here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that, but:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks for explaining the default-log-volume concern.
I changed the approach in d5e680d. Instead of adding a second Info-level line for tunnel close, the request log is now emitted after request completion and includes the final status and duration. So the default Info output still stays at one access-log-style line per request, but CONNECT requests now also carry the completion information that was missing before.
For example, a CONNECT tunnel now logs as a single line like:
Request: <remote> => <local> "<user>" HTTP/1.1 CONNECT example.com:443 200 OK dur=2m15sRegular HTTP proxy requests use the same shape and are also logged after the response body forwarding completes. I think this better matches the Apache/nginx access log model you described while still making CONNECT duration visible without requiring a separate close lifecycle message.
Tests:
go test ./...