-
Notifications
You must be signed in to change notification settings - Fork 80
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
Quieter HTTP 206 logging: #550
Merged
jacobweinstock
merged 21 commits into
tinkerbell:main
from
jacobweinstock:quieter-logging-for-iso
Nov 21, 2024
Merged
Quieter HTTP 206 logging: #550
jacobweinstock
merged 21 commits into
tinkerbell:main
from
jacobweinstock:quieter-logging-for-iso
Nov 21, 2024
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
===================================
Coverage 49% 50%
===================================
Files 27 27
Lines 2951 2975 +24
===================================
+ Hits 1461 1502 +41
+ Misses 1447 1421 -26
- Partials 43 52 +9 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
2 times, most recently
from
November 18, 2024 18:49
753dc2d
to
ef7a651
Compare
rahulbabu95
reviewed
Nov 18, 2024
rahulbabu95
reviewed
Nov 18, 2024
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
from
November 18, 2024 23:38
a309391
to
1befe8a
Compare
jacobweinstock
changed the title
Clean up and make logging quieter:
Quieter HTTP 206 logging:
Nov 19, 2024
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
2 times, most recently
from
November 19, 2024 21:42
c973dc0
to
b2279f9
Compare
rahulbabu95
reviewed
Nov 20, 2024
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
2 times, most recently
from
November 20, 2024 20:20
6a07cea
to
c3fb60b
Compare
Logging all 206 partial content requests from the middleware package creates a lot of messages per ISO request. This stops the middleware logger from logging anything and lets the ISO http func handler decide what to log. In the happy path case we only log that patching happened successfully. Signed-off-by: Jacob Weinstock <[email protected]>
This library allows for sampling log messages that match some certain criteria. I have not implemented this in Smee, but this could allow for sampling each unique mac address in the url path. This would be as opposed to having all traffic be sampled together. This needs tested to see if it's worth it. If it's not there are other ways to do the sampling in the same way without needing to import a library. Signed-off-by: Jacob Weinstock <[email protected]>
Remove all old manifests and use Tilt to deploy the stack via Helm and rebuild Smee and deploy a new image on changes. Signed-off-by: Jacob Weinstock <[email protected]>
There was a merge conflict and I removed needed go.sum lines. This adds them back. Signed-off-by: Jacob Weinstock <[email protected]>
The sampling import isn't needed for now. The most simple case of using a random number below a percentage will suffice for now. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
This is needed for clients that request the entire ISO in one request as opposed to the 206 partial content requests. Signed-off-by: Jacob Weinstock <[email protected]>
This creates a test ISO, patches it and validates the patch. I need to clean it up a bit though. Signed-off-by: Jacob Weinstock <[email protected]>
This library seems more maintained and allows for reading files in an ISO without having to mount it to the disk. Signed-off-by: Jacob Weinstock <[email protected]>
This adds logging output to the test results and makes them more difficult to read. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
I was just ignoring the error, so an error is now just returned as 0. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
This allows us to avoid having to write a file to disk in a unit test. Signed-off-by: Jacob Weinstock <[email protected]>
This is a work in progress to handle ranges that could cause the patching to read into memory large chunk sizes. This could cause OOM killing and/or DOS the service. Signed-off-by: Jacob Weinstock <[email protected]>
The live update makes the development loop quite fast. Signed-off-by: Jacob Weinstock <[email protected]>
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
from
November 21, 2024 07:00
c3fb60b
to
4b18a16
Compare
If the request is a partial content request, we need to validate the Content-Range header. Because we read the entire response body into memory for patching, we need to ensure that the Content-Range is within a reasonable size. For now, we are limiting the size to 500Kb. Signed-off-by: Jacob Weinstock <[email protected]>
Remove reference to my local machine. Signed-off-by: Jacob Weinstock <[email protected]>
Signed-off-by: Jacob Weinstock <[email protected]>
jacobweinstock
force-pushed
the
quieter-logging-for-iso
branch
from
November 21, 2024 18:36
1000e95
to
edd662d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Logging all 206 partial content requests from the middleware package creates a lot of messages per ISO request. In testing this was around 3000 requests. This is too many logs and will create too much noise.
There is an HTTP middleware in Smee that logs a single line for all requests. This PR stops the middleware logger from logging anything when the
X-Global-Logging
header is set in the HTTP response. This allows HTTP handler functions to decide if middleware logging should occur or not.For the ISO HTTP handler function we disable the middleware logging and only sample the HTTP 206 request messages. The following is an example output from an ISO mount using the 0.002% sampling rate. My estimate is that this sampling rate us about 5 - 10 log messages per ISO mount.
The first 8 lines are from when the system is booting up the ISO mounted as a virtual CDROM/DVD. The last 3 lines are from when HookOS boots up and mounts the ISO as a CDROM device.
Also added is some checking/validation around content-size. A max content size of 512Kb is implemented.
And finally, the Tiltfile was updated and implements live updates. Any associated kustomize directories and files were removed and deploying Smee is done via the stack helm chart.
Why is this needed
Fixes: #
How Has This Been Tested?
How are existing users impacted? What migration steps/scripts do we need?
Checklist:
I have: