application_logs: add bootstrap option to write logs in JSON format

[ohadvano]

Commit Message: application_logs: add bootstrap option to write logs in JSON format
Additional Description: Adds an option in bootstrap config to write application logs in JSON format, while supporting all the log-format flags as defined in the CLI --log-format option. Related to #25959 - this is the first step in the implementation for supporting custom JSON properties, while printing the application logs output in JSON format.
Risk Level: Low (all new code paths are only enabled by config option)
Testing: Unit tests
Docs Changes: API, Application logs docs
Release Notes: None
Platform Specific Features: None

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27278 was opened by ohadvano.

see: more, trace.

[ohadvano]

cc @mattklein123 as you engaged in the issue thread, will appreciate your thoughts on this, thanks.

Example usage:

application_log_format:
  json_format:
    ProcessId: "%P"
    Message: "%v"
    Level: "%l"
    Time: "%Y-%m-%d %T.%e"

Expected output:
{"Time":"2023-05-09T14:03:50.753171170","ProcessId":"1010","Level":"info","Message":"my_message"}

Also asking for some guidance on an implementation problem I see. It seems like the logger is created before the server is initialized and therefore before the bootstrap config is loaded. This means that the above config would not be available at the time of log creation.

I can think of either:

1. Late setting of the log format (but then some of the logs would be written in the default log format)
2. Refactoring or reading the yaml config before the logger is created, either by moving the config parse to an earlier step of the bootstrap, or reading and parsing it twice (where the first time would be solely for configuring the log) - after going through the code, I don't believe this is reasonable, especially since the Apl::Impl would need to move earlier, and it seems too risky for the cause.

IMO option 1 sounds reasonable, if it's explicitly specified in the docs that some of the initialization logs would be written in the default log format (or the one specified by --log-format CLI option), even if application_log_json is configured in the yaml.

[ohadvano]

I added implementation based on the first approach (see previous comment). Will appreciate initial review before proceeding to unit tests, to make sure this is on track.

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @snowp

🐱

Caused by: a #27278 (comment) was created by @ohadvano.

see: more, trace.

[adisuissa]

Thanks!
At a high level API looks good other than a few nits.
Left a few minor code comments.

[ohadvano]

@adisuissa/ @snowp / @mattklein123 the PR is ready for review, thanks

[ohadvano]

/coverage

[repokitteh-read-only]

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/27278/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #27278 (comment) was created by @ohadvano.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @alyssawilk

🐱

Caused by: #27278 was synchronize by ohadvano.

see: more, trace.

[mattklein123]

Awesome, thanks!

[mattklein123]

LGTM. Not sure if @alyssawilk has any other comments so I will let her merge.

[alyssawilk]

thanks for swapping to disallow both configs

I'm mildly concerned this PR is going to silently break the mobile build due to CI being set up correctly - there's a new build option on the E-M side I think this PR will tickle, and I'm not seeing the tests run.

I'm trying to fix upstream CI to run correctly over in #27707 but as an interim workaround if you don't want to wait, you could do the horrible hacky workaround of editing a file (reword a comment in mobile/test/common/integration/) and see if this breaks the mobile compile time options build. sorry for the hassle but it beats a revert!

/wait

[alyssawilk]

looks fine. fingers crossed then :-)

[ohadvano]

/retest

[alyssawilk]

very likely a flake - retest will verify. a few of the mobile CI builds especially the ones that pull external dependencies have network errors, which is one of the reasons we don't run them on all CI :-)
the only one I was concerned about was cc_tests in compile time options so the PR is in the submit queue and retest should land it CI willing :-)

[ohadvano]

/retest

[phlax]

@alyssawilk @ohadvano this failed clang-tidy and was merged anyway afaict https://dev.azure.com/cncf/envoy/_build/results?buildId=139360&view=logs&j=b7634614-24f3-5416-e791-4f3affaaed6c&t=d565ee52-b888-5d80-7fea-dfaed104c390&l=167

[ohadvano]

@phlax nope, all CI steps passed: https://dev.azure.com/cncf/envoy/_build/results?buildId=139347&view=results
Any chance the clang-tidy checks were changed recently and wasn't rebased into the PR branch?

[phlax]

Any chance the clang-tidy checks were changed recently and wasn't rebased into the PR branch?

the clang tidy test is not great - it uses a diff, takes ages, and seems to miss stuff so its quite possible that for some reason it didnt detect the change correctly and report the issue

either way it failed on postsubmit (ie genuinely failed and should be fixed)

it also triggered an asan issue which i was just trying to track and cant see any previous occurrence https://dev.azure.com/cncf/envoy/_build/results?buildId=139360&view=logs&j=1439b9f7-a348-5b50-b5fe-ea612ea91241&t=20bd2bcd-3fb5-5bf3-9cdb-ca6f94dd3096&l=2209 - altho this test flakes pretty regularly so not sure at all about that

in the case of the asan test im guessing its just triggered something #27814