Set envoy log level per pod #1849
Conversation
cli/cmd/proxy/envoy/http.go
Outdated
| // EnvoyConfig represents the configuration retrieved from a config dump at the | ||
| // admin endpoint. It wraps the Envoy ConfigDump struct to give us convenient | ||
| // access to the different sections of the config. | ||
| type EnvoyConfig struct { | ||
| rawCfg []byte | ||
| RawCfg []byte |
There was a problem hiding this comment.
so this change and changing the tests to utilize the testdata directory rather than embedding are the only substantial changes here other than the move
| @@ -222,7 +227,7 @@ func (l *LogLevelCommand) fetchAdminPorts() (map[string]int, error) { | |||
| return adminPorts, nil | |||
| } | |||
|
|
|||
| func (l *LogLevelCommand) fetchLogLevels(adminPorts map[string]int) (map[string]LoggerConfig, error) { | |||
| func (l *LogLevelCommand) fetchOrSetLogLevels(adminPorts map[string]int) error { | |||
There was a problem hiding this comment.
so I also consolidated the calls to envoy into this function, because they were both the exact same code, I'd prefer to keep them separate so it reads better, but having the envoy API is kinda forcing my hand to keep this code from being completely duplicated
jm96441n
changed the title
jm96441n
force-pushed
the
set-envoy-log-level-per-pod
branch
from
f3ba599
to
587f91c
Compare
There was a problem hiding this comment.
So, cursory pass and the code looks generally fine to me, but I guess I have a question/observation about the usability and/or flag naming conventions. It seems a bit confusing to me that what appears to be the "listing" log-level command also acts as double-duty for setting said log-levels.
If we're fine going with that approach, what do you think about making the flag name a bit more specific? Saying -level or -l means we don't really have any user-indication that they're actually doing some sort of "set" command. What about something like -update-level or -u?
|
I like the idea of using |
Move format parsing into envoy package Move enovy to common package, move param parsing to calling package use a LoggerParams struct for handling a format for log changes to envoy refactor to use logger params and methods to set and validate logger and log levels before calling envoy linting changes clean up from rebase Improve comment on envoy logging endpoint function, switched to using '-update-level' for updating envoy log level flag for better usability
jm96441n
force-pushed
the
set-envoy-log-level-per-pod
branch
from
33a884f
to
de88a41
Compare
Changes proposed in this PR:
consul-k8s proxy log POD_NAME -l warning, or on specific loggers byconsul-k8s proxy log POD_NAME -l grpc:warningorconsul-k8s proxy log POD_NAME -l grpc:warning,http:infoenvoypackage to encapsulate interactions with envoyHow I've tested this PR:
kind create clusterconsul-values.ymlfile in the above gist:consul-k8s install -config-file=consul-values.yaml -set global.image=hashicorp/consul:1.14.3server.yamlto your clusterkubectl apply server.yamlkubectl get podskubectl port-forward <POD_NAME> 19000:19000(where19000is the default admin port for the envoy proxy)consul-k8s proxy log <POD_NAME> -l warningand compare the output to the envoy admin interface which would be atlocalhost:19000/loggingwhile port forwarding is enabled (rinse and repeat for other combinations of changing levels)How I expect reviewers to test this PR:
code review
potentially running above steps (but probably not necessary)
NOTE: Didn't add a CHANGELOG yet, planning on adding the CHANGELOG entry once the work on the consul-k8s proxy log command is complete, though can add one for the current work if that's preferred (the next PR should be the last one to add reset functionality to put logs back to the default level for envoy)
Checklist: