Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/envoy/config/bootstrap/v3/bootstrap.proto
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ message Bootstrap {
// support all the format flags specified in the :option:`--log-format`
// command line options section, except for the ``%v`` and ``%_`` flags.
google.protobuf.Struct json_format = 1;

// Flush application log in a format defined by a string. The text format
// can support all the format flags specified in the :option:`--log-format`
// command line option section.
string text_format = 2;
}
}

Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ new_features:
Added bootstrap option
:ref:`application_log_format <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.ApplicationLogConfig.LogFormat.json_format>`
to enable setting application log format as JSON structure.
- area: application_logs
change: |
Added bootstrap option
:ref:`application_log_format <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.ApplicationLogConfig.LogFormat.text_format>`
to enable setting application log text format from config.
- area: ext_proc
change: |
added new field ``filter_metadata <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExtProc.filter_metadata`` to aid in logging.
Expand Down
9 changes: 7 additions & 2 deletions source/server/utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ void assertExclusiveLogFormatMethod(

void maybeSetApplicationLogFormat(
const envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig& application_log_config) {
if (application_log_config.has_log_format() &&
application_log_config.log_format().has_json_format()) {
if (!application_log_config.has_log_format()) {
return;
}

if (application_log_config.log_format().has_text_format()) {
Logger::Registry::setLogFormat(application_log_config.log_format().text_format());
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone sets the CLI option? I think this is similar to the other convo we had? Should we have an error if someone sets the CLI option and then sets the bootstrap option? I admit that perhaps there is a use case where someone could set the log format before the bootstrap is read, but that seems like kind of an edge case and we could just see if anyone complains about that? wdyt?

Copy link
Contributor Author

@ohadvano ohadvano Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - if we decided to follow some pattern then I think it should be applied to all cases.
The exclusive format method assertion function is called before stepping into this function, in:

Utility::assertExclusiveLogFormatMethod(options_, bootstrap_.application_log_config());

So this case should be covered, and any other log format that would be added in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks

} else if (application_log_config.log_format().has_json_format()) {
const auto status =
Logger::Registry::setJsonLogFormat(application_log_config.log_format().json_format());

Expand Down
41 changes: 41 additions & 0 deletions test/server/config_validation/server_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,23 @@ class JsonApplicationLogsValidationServerForbiddenFlag_Test : public ValidationS
}
};

class TextApplicationLogsValidationServerTest : public ValidationServerTest {
public:
static void SetUpTestSuite() { // NOLINT(readability-identifier-naming)
setupTestDirectory();
}

static void setupTestDirectory() {
directory_ =
TestEnvironment::runfilesDirectory("envoy/test/server/config_validation/test_data/");
}

static const std::vector<std::string> getAllConfigFiles() {
setupTestDirectory();
return {"text_application_logs.yaml"};
}
};

TEST_P(ValidationServerTest, Validate) {
EXPECT_TRUE(validateConfig(options_, Network::Address::InstanceConstSharedPtr(),
component_factory_, Thread::threadFactoryForTest(),
Expand Down Expand Up @@ -324,6 +341,30 @@ INSTANTIATE_TEST_SUITE_P(
::testing::ValuesIn(
JsonApplicationLogsValidationServerForbiddenFlag_Test::getAllConfigFiles()));

TEST_P(TextApplicationLogsValidationServerTest, TextApplicationLogs) {
Thread::MutexBasicLockable access_log_lock;
Stats::IsolatedStoreImpl stats_store;
DangerousDeprecatedTestTime time_system;
ValidationInstance server(options_, time_system.timeSystem(),
Network::Address::InstanceConstSharedPtr(), stats_store,
access_log_lock, component_factory_, Thread::threadFactoryForTest(),
Filesystem::fileSystemForTest());

Envoy::Logger::Registry::setLogLevel(spdlog::level::info);
MockLogSink sink(Envoy::Logger::Registry::getSink());
EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) {
EXPECT_THAT(msg, HasSubstr("[lvl: info][msg: hello]"));
EXPECT_EQ(log.logger_name, "misc");
}));

ENVOY_LOG_MISC(info, "hello");
server.shutdown();
}

INSTANTIATE_TEST_SUITE_P(
AllConfigs, TextApplicationLogsValidationServerTest,
::testing::ValuesIn(TextApplicationLogsValidationServerTest::getAllConfigFiles()));

} // namespace
} // namespace Server
} // namespace Envoy
10 changes: 10 additions & 0 deletions test/server/config_validation/test_data/text_application_logs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
application_log_config:
log_format:
text_format: "[lvl: %l][msg: %v]"

admin:
address:
socket_address:
address: 0.0.0.0
port_value: 9000
13 changes: 13 additions & 0 deletions test/server/server_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,19 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlag_) {
"setJsonLogFormat error: INVALID_ARGUMENT: Usage of %_ is unavailable for JSON log formats");
}

TEST_P(ServerInstanceImplTest, TextApplicationLog) {
EXPECT_NO_THROW(initialize("test/server/test_data/server/text_application_log.yaml"));

Envoy::Logger::Registry::setLogLevel(spdlog::level::info);
MockLogSink sink(Envoy::Logger::Registry::getSink());
EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) {
EXPECT_THAT(msg, HasSubstr("[lvl: info][msg: hello]"));
EXPECT_EQ(log.logger_name, "misc");
}));

ENVOY_LOG_MISC(info, "hello");
}

} // namespace
} // namespace Server
} // namespace Envoy
10 changes: 10 additions & 0 deletions test/server/test_data/server/text_application_log.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
application_log_config:
log_format:
text_format: "[lvl: %l][msg: %v]"

admin:
address:
socket_address:
address: 0.0.0.0
port_value: 9000
6 changes: 6 additions & 0 deletions test/server/utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ TEST(UtilsTest, MaybeSetApplicationLogFormat) {
EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(log_config));
}

{
envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config;
log_config.mutable_log_format()->mutable_text_format();
EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(log_config));
}

{
envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config;
auto* format = log_config.mutable_log_format()->mutable_json_format();
Expand Down