[#369] Enhanced http requests logging #426
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#426
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-s3-gw:feature/9613-enhanced_log"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com
WIP: [#369] Enhanced http requests loggingto WIP: [#369] Enhanced http requests logging@ -0,0 +29,4 @@
httplog = withFieldIfExist(httplog, "query", r.URL.Query())
httplog = withFieldIfExist(httplog, "headers", r.Header)
if r.ContentLength != 0 && r.ContentLength <= settings.MaxBody {
Sometimes
r.ContentLength
can be-1
that means unknown length. Now we will try read body and get panic inwithBody
when try to allocate new buffervar body = make([]byte, r.ContentLength)
.Should we ignore such body or log first
settings.MaxBody
bytes anyway?@ -0,0 +44,4 @@
func withBody(httplog *zap.Logger, r *http.Request) (*zap.Logger, error) {
var body = make([]byte, r.ContentLength)
_, err := r.Body.Read(body)
We need close body after read.
Besides, we must preserve initial body in request. After read the body doesn't contain anything and in handlers we get errors. For example:
@ -129,6 +130,7 @@ type Config struct {
func NewRouter(cfg Config) *chi.Mux {
api := chi.NewRouter()
api.Use(
s3middleware.LogHTTP(cfg.Log, cfg.LogHTTP),
If enabling this middleware being configured only once (and don't support SIGHUP reload) we can add it conditionally (like
s3middleware.FrostfsIDValidation
)@ -75,6 +75,9 @@ const ( // Settings.
cfgLoggerLevel = "logger.level"
cfgLoggerDestination = "logger.destination"
cfgLoggerRequestEnabled = "http_logging.enabled"
We should mention this params in
config/config.env
anddocs/configuration.md
We should mention this params in
config/config.env
anddocs/configuration.md
@ -76,2 +76,4 @@
cfgLoggerDestination = "logger.destination"
cfgLoggerRequestEnabled = "http_logging.enabled"
cfgLoggerRequestMaxBody = "http_logging.max_body"
We should mention this params in
config/config.env
anddocs/configuration.md
@ -142,3 +103,1 @@
CouldntCacheSubject = "couldn't cache subject info"
UserGroupsListIsEmpty = "user groups list is empty, subject not found"
CouldntCacheUserKey = "couldn't cache user key"
Empty line
95c2fa7fda
toea51fe6e4b
Overall looks quite pretty. I suppose we can merge it in feature branch after comment fixes.
@ -0,0 +42,4 @@
return func(h http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var err error
config.fileLogger, err = getFileLogger(config)
It looks quite odd for me to see
config
field initialized by the function, that reads config as a parameter. EitherfilerLogger
should not be a part of config structure orgetFileLogger
should be defined as function on config struct. Prefer making pure functions when it is possible and reasonable.I suggest to do something like
@ -0,0 +112,4 @@
return err
}
config.sinkPath = SinkName + ":" + pth
As previous comment mentioned, try to make this function pure by returing
sinkPath
instead of config modification. This pattern is valid overall but it doesn't make sense here.@ -376,0 +389,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|----------------|--------|---------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `enabled` | bool | yes | false | |
| `max_body` | int | yes | 1024 | Max body size for log output. |
Is it bytes, kilobytes, megabytes?
@ -376,0 +390,4 @@
|----------------|--------|---------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `enabled` | bool | yes | false | |
| `max_body` | int | yes | 1024 | Max body size for log output. |
| `max_log_size` | int | yes | 50 | Log file size threshold (in Megabytes) to be moved in backup file. |
Where are backup files are stored? Is it in destination dir?
@ -376,0 +392,4 @@
| `max_body` | int | yes | 1024 | Max body size for log output. |
| `max_log_size` | int | yes | 50 | Log file size threshold (in Megabytes) to be moved in backup file. |
| `gzip` | bool | yes | false | Whether to enable Gzip compression for backup log files. |
| `destination` | string | yes | stdout | Specify path for log output. Accepts path string (maybe even URL - not tested) to log in file, or "stdout" and "stderr" reserved phrases for printing in output stream. |
Do not mention URL if we didn't test it. 😃 Also, can we test it? What do we need to do that?
Also, I suppose
destination
is a path to dir, not a file, am I right?destination
specifies path with filename e.g. /var/log/http.log. During backup the filename is added with timestamp. And a new file with initial name creates.I will append this to configuration.md
ea51fe6e4b
to7e6212264d
7e6212264d
toe358e0d8b9
e358e0d8b9
to6f3bedc023
WIP: [#369] Enhanced http requests loggingto [#369] Enhanced http requests logging