Add option to suppress deprecated warning logs. #37407
Conversation
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/retest |
|
/lgtm api |
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
vamshi177
force-pushed
the
skip-logs-cli
branch
from
c5e61ab to
f56162b
Compare
There was a problem hiding this comment.
Thanks!
A high-level comment: I sometimes wonder whether it is better to have a bootstrap knob to set these kinds of configurations (instead of a CLI option). The reason is that it may be easier to have a consistent config tracked in a file rather than in a CLI argument. The downside is probably that this feature may not kick in until the bootstrap was validated, and deprecated messages would have been spitted out. I'm good with the current proposal, just raising something to think about.
Leaving it up to @wbpcode to decide as a senior-maintainer.
test/integration/server.h
Outdated
| @@ -40,6 +40,7 @@ struct FieldValidationConfig { | |||
| bool allow_unknown_static_fields = false; | |||
| bool reject_unknown_dynamic_fields = false; | |||
| bool ignore_unknown_dynamic_fields = false; | |||
| bool suppress_deprecated_warning_logs = false; | |||
There was a problem hiding this comment.
I don't think this is being used, and can probably be removed (the call in server.cc can be with false).
There was a problem hiding this comment.
removed the suppress_deprecated_warning_logs variable and directly updated the call in server.cc with false.
test/mocks/server/options.h
Outdated
| @@ -64,6 +65,7 @@ class MockOptions : public Options { | |||
| bool allow_unknown_static_fields_{}; | |||
| bool reject_unknown_dynamic_fields_{}; | |||
| bool ignore_unknown_dynamic_fields_{}; | |||
| bool suppress_deprecated_warning_logs_{}; | |||
There was a problem hiding this comment.
I don't think this is being used, and can probably be removed (the default return value of the mock can be false, and a test can override it if needed).
There was a problem hiding this comment.
removed this variable and directly updated the mock to return false.
We initially considered bootstrap option but pivoted to command line for the exact reason that you mentioned. |
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
|
Thanks for the initial review. I have updated the tests based on the comments. |
api/envoy/admin/v3/server_info.proto
Outdated
| // See :option:`--suppress-deprecated-warning-logs` for details. | ||
| bool suppress_deprecated_warning_logs = 41; |
There was a problem hiding this comment.
Thanks so much for this contribution. I am good to the implementation and code. But I personally prefer short command line option name at the precondition that without compromising the meaning.
So, I will prefer no_deprecated_warning or someting. WDYT?
/wait-any
There was a problem hiding this comment.
how about skip_deprecated_logs ?
There was a problem hiding this comment.
Thanks for the review.
will update the command line option to more shorter name like skip_deprecated_logs.
There was a problem hiding this comment.
Updated the code with a shorter command line option skip_deprecated_logs that retains the same meaning.
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
|
/retest |
|
Could merge main and resolve the conflicts? Thanks. |
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
|
/retest |
Signed-off-by: G Vamshi Krishna Reddy <[email protected]>
|
cc @adisuissa |
Commit Message: Add option to suppress deprecated warning logs.
Additional Description:
This commit introduces a new command-line option
--suppress-deprecated-warning-logsthat allows users to suppress log messages related to deprecated fields during Protobuf message validation. When enabled, Envoy will no longer log warnings for deprecated fields, which is useful for reducing log verbosity in production environments. By default, deprecated field warnings are logged as usual.Risk Level: Low
Testing: The new option was verified by running Envoy with a static configuration file. When
--suppress-deprecated-warning-logswas enabled, deprecated warning messages were suppressed from the logs, as expected. Tests also confirm correct log behavior when the option is toggled.Docs Changes: yes
Release Notes: yes
Fixes: Addresses issue of reducing log verbosity in production environments as discussed at #37134
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]