From 171c36527455c2709be57f2ccc41218d45689411 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Wed, 6 Dec 2017 17:29:32 -0800 Subject: [PATCH] logging: allow partial updates to log handler settings Summary: This updates the LogHandlerConfig code to allow changing the settings on an existing log handler without listing all of its existing options from scratch. This also changes the syntax of the basic log handler configuration string to use a colon to separate the log handler name+type from the options list. In other words, rather than specifying `default=stream,stream=stderr,async=true` you now say `default=stream:stream=stderr,async=true`. The primary motivation for this change is to make it easy for users to switch the async setting for the default log handler on or off. Callers can now specify `default:async=true` to easily enable async mode on the default log handler without having to completely re-list the full settings for the default handler. Reviewed By: yfeldblum Differential Revision: D6494228 fbshipit-source-id: 52a296f800a5456f0c3aa10546298139c8db52fc --- folly/experimental/logging/Init.cpp | 2 +- folly/experimental/logging/LogConfig.cpp | 23 +++- .../experimental/logging/LogConfigParser.cpp | 91 ++++++++++---- .../experimental/logging/LogHandlerConfig.cpp | 21 ++++ folly/experimental/logging/LogHandlerConfig.h | 26 +++- folly/experimental/logging/LoggerDB.cpp | 43 +++++-- folly/experimental/logging/docs/Config.md | 41 ++++-- .../logging/test/ConfigParserTest.cpp | 96 +++++++++++--- .../logging/test/ConfigUpdateTest.cpp | 118 +++++++++++++----- folly/experimental/logging/test/fatal_test.py | 4 +- 10 files changed, 363 insertions(+), 102 deletions(-) diff --git a/folly/experimental/logging/Init.cpp b/folly/experimental/logging/Init.cpp index 24600510..73542a8e 100644 --- a/folly/experimental/logging/Init.cpp +++ b/folly/experimental/logging/Init.cpp @@ -44,7 +44,7 @@ namespace folly { * handler, the handler will be automatically forgotten by the LoggerDB code. */ constexpr StringPiece kDefaultLoggingConfig = - ".=WARN:default; default=stream,stream=stderr,async=false"; + ".=WARN:default; default=stream:stream=stderr,async=false"; void initLogging(StringPiece configString) { // Register the StreamHandlerFactory diff --git a/folly/experimental/logging/LogConfig.cpp b/folly/experimental/logging/LogConfig.cpp index a622f281..7a35c9ad 100644 --- a/folly/experimental/logging/LogConfig.cpp +++ b/folly/experimental/logging/LogConfig.cpp @@ -15,6 +15,8 @@ */ #include +#include + namespace folly { bool LogConfig::operator==(const LogConfig& other) const { @@ -30,9 +32,24 @@ void LogConfig::update(const LogConfig& other) { // Update handlerConfigs_ with all of the entries from the other LogConfig. // Any entries already present in our handlerConfigs_ are replaced wholesale. for (const auto& entry : other.handlerConfigs_) { - auto result = handlerConfigs_.insert(entry); - if (!result.second) { - result.first->second = entry.second; + if (entry.second.type.hasValue()) { + // This is a complete LogHandlerConfig that should be inserted + // or completely replace an existing handler config with this name. + auto result = handlerConfigs_.insert(entry); + if (!result.second) { + result.first->second = entry.second; + } + } else { + // This config is updating an existing LogHandlerConfig rather than + // completely replacing it. + auto iter = handlerConfigs_.find(entry.first); + if (iter == handlerConfigs_.end()) { + throw std::invalid_argument(to( + "cannot update configuration for unknown log handler \"", + entry.first, + "\"")); + } + iter->second.update(entry.second); } } diff --git a/folly/experimental/logging/LogConfigParser.cpp b/folly/experimental/logging/LogConfigParser.cpp index 5eb805ac..ac9f5bdc 100644 --- a/folly/experimental/logging/LogConfigParser.cpp +++ b/folly/experimental/logging/LogConfigParser.cpp @@ -357,49 +357,84 @@ bool splitNameValue( } std::pair parseHandlerConfig(StringPiece value) { - std::vector pieces; - folly::split(",", value, pieces); - FOLLY_SAFE_DCHECK( - pieces.size() >= 1, "folly::split() always returns a list of length 1"); + // Parse the handler name and optional type + auto colonIndex = value.find(':'); + StringPiece namePortion; + StringPiece optionsStr; + if (colonIndex == StringPiece::npos) { + namePortion = value; + } else { + namePortion = StringPiece{value.begin(), value.begin() + colonIndex}; + optionsStr = StringPiece{value.begin() + colonIndex + 1, value.end()}; + } StringPiece handlerName; - StringPiece handlerType; - if (!splitNameValue(pieces[0], &handlerName, &handlerType)) { - throw LogConfigParseError{to( - "error parsing log handler configuration \"", - pieces[0], - "\": expected data in the form NAME=TYPE")}; + Optional handlerType(in_place); + if (!splitNameValue(namePortion, &handlerName, &handlerType.value())) { + handlerName = trimWhitespace(namePortion); + handlerType = folly::none; } + + // Make sure the handler name and type are not empty. + // Also disallow commas in the name: this helps catch accidental errors where + // the user left out the ':' and intended to be specifying options instead of + // part of the name or type. if (handlerName.empty()) { throw LogConfigParseError{ "error parsing log handler configuration: empty log handler name"}; } - if (handlerType.empty()) { + if (handlerName.contains(',')) { throw LogConfigParseError{to( "error parsing configuration for log handler \"", handlerName, - "\": empty log handler type")}; + "\": name cannot contain a comma when using the basic config format")}; } - - LogHandlerConfig config{handlerType}; - for (size_t n = 1; n < pieces.size(); ++n) { - StringPiece optionName; - StringPiece optionValue; - if (!splitNameValue(pieces[n], &optionName, &optionValue)) { + if (handlerType.hasValue()) { + if (handlerType->empty()) { throw LogConfigParseError{to( "error parsing configuration for log handler \"", handlerName, - "\": options must be of the form NAME=VALUE")}; + "\": empty log handler type")}; } - - auto ret = config.options.emplace(optionName.str(), optionValue.str()); - if (!ret.second) { + if (handlerType->contains(',')) { throw LogConfigParseError{to( "error parsing configuration for log handler \"", handlerName, - "\": duplicate configuration for option \"", - optionName, - "\"")}; + "\": invalid type \"", + handlerType.value(), + "\": type name cannot contain a comma when using " + "the basic config format")}; + } + } + + // Parse the options + LogHandlerConfig config{handlerType}; + optionsStr = trimWhitespace(optionsStr); + if (!optionsStr.empty()) { + std::vector pieces; + folly::split(",", optionsStr, pieces); + FOLLY_SAFE_DCHECK( + pieces.size() >= 1, "folly::split() always returns a list of length 1"); + + for (const auto& piece : pieces) { + StringPiece optionName; + StringPiece optionValue; + if (!splitNameValue(piece, &optionName, &optionValue)) { + throw LogConfigParseError{to( + "error parsing configuration for log handler \"", + handlerName, + "\": options must be of the form NAME=VALUE")}; + } + + auto ret = config.options.emplace(optionName.str(), optionValue.str()); + if (!ret.second) { + throw LogConfigParseError{to( + "error parsing configuration for log handler \"", + handlerName, + "\": duplicate configuration for option \"", + optionName, + "\"")}; + } } } @@ -535,7 +570,11 @@ dynamic logConfigToDynamic(const LogHandlerConfig& config) { for (const auto& opt : config.options) { options.insert(opt.first, opt.second); } - return dynamic::object("type", config.type)("options", options); + auto result = dynamic::object("options", options); + if (config.type.hasValue()) { + result("type", config.type.value()); + } + return std::move(result); } dynamic logConfigToDynamic(const LogCategoryConfig& config) { diff --git a/folly/experimental/logging/LogHandlerConfig.cpp b/folly/experimental/logging/LogHandlerConfig.cpp index 009f8207..74fe05d9 100644 --- a/folly/experimental/logging/LogHandlerConfig.cpp +++ b/folly/experimental/logging/LogHandlerConfig.cpp @@ -15,13 +15,34 @@ */ #include +#include + +using std::string; + namespace folly { +LogHandlerConfig::LogHandlerConfig() {} + LogHandlerConfig::LogHandlerConfig(StringPiece t) : type{t.str()} {} +LogHandlerConfig::LogHandlerConfig(Optional t) + : type{t.hasValue() ? Optional{t->str()} : Optional{}} {} + LogHandlerConfig::LogHandlerConfig(StringPiece t, Options opts) : type{t.str()}, options{std::move(opts)} {} +LogHandlerConfig::LogHandlerConfig(Optional t, Options opts) + : type{t.hasValue() ? Optional{t->str()} : Optional{}}, + options{std::move(opts)} {} + +void LogHandlerConfig::update(const LogHandlerConfig& other) { + FOLLY_SAFE_DCHECK( + !other.type.hasValue(), "LogHandlerConfig type cannot be updated"); + for (const auto& option : other.options) { + options[option.first] = option.second; + } +} + bool LogHandlerConfig::operator==(const LogHandlerConfig& other) const { return type == other.type && options == other.options; } diff --git a/folly/experimental/logging/LogHandlerConfig.h b/folly/experimental/logging/LogHandlerConfig.h index 4fe3a3fe..698d981b 100644 --- a/folly/experimental/logging/LogHandlerConfig.h +++ b/folly/experimental/logging/LogHandlerConfig.h @@ -18,6 +18,7 @@ #include #include +#include #include namespace folly { @@ -29,13 +30,32 @@ class LogHandlerConfig { public: using Options = std::unordered_map; - explicit LogHandlerConfig(folly::StringPiece type); - LogHandlerConfig(folly::StringPiece type, Options options); + LogHandlerConfig(); + explicit LogHandlerConfig(StringPiece type); + explicit LogHandlerConfig(Optional type); + LogHandlerConfig(StringPiece type, Options options); + LogHandlerConfig(Optional type, Options options); + + /** + * Update this LogHandlerConfig object by merging in settings from another + * LogConfig. + * + * The other LogHandlerConfig must not have a type set. + */ + void update(const LogHandlerConfig& other); bool operator==(const LogHandlerConfig& other) const; bool operator!=(const LogHandlerConfig& other) const; - std::string type; + /** + * The handler type name. + * + * If this field is unset than this configuration object is intended to be + * used to update an existing LogHandler object. This field must always + * be set in the configuration for all existing LogHandler objects. + */ + Optional type; + Options options; }; diff --git a/folly/experimental/logging/LoggerDB.cpp b/folly/experimental/logging/LoggerDB.cpp index 1f58c047..bd64d257 100644 --- a/folly/experimental/logging/LoggerDB.cpp +++ b/folly/experimental/logging/LoggerDB.cpp @@ -195,13 +195,6 @@ void LoggerDB::startConfigUpdate( // Create all of the new LogHandlers needed from this configuration for (const auto& entry : config.getHandlerConfigs()) { - // Look up the LogHandlerFactory - auto factoryIter = handlerInfo->factories.find(entry.second.type); - if (factoryIter == handlerInfo->factories.end()) { - throw std::invalid_argument(to( - "unknown log handler type \"", entry.second.type, "\"")); - } - // Check to see if there is an existing LogHandler with this name std::shared_ptr oldHandler; auto iter = handlers->find(entry.first); @@ -209,17 +202,49 @@ void LoggerDB::startConfigUpdate( oldHandler = iter->second; } + LogHandlerConfig updatedConfig; + const LogHandlerConfig* handlerConfig; + if (entry.second.type.hasValue()) { + handlerConfig = &entry.second; + } else { + // This configuration is intended to update an existing LogHandler + if (!oldHandler) { + throw std::invalid_argument(to( + "cannot update unknown log handler \"", entry.first, "\"")); + } + + updatedConfig = oldHandler->getConfig(); + if (!updatedConfig.type.hasValue()) { + // This normally should not happen unless someone improperly manually + // constructed a LogHandler object. All existing LogHandler objects + // should indicate their type. + throw std::invalid_argument(to( + "existing log handler \"", + entry.first, + "\" is missing type information")); + } + updatedConfig.update(entry.second); + handlerConfig = &updatedConfig; + } + + // Look up the LogHandlerFactory + auto factoryIter = handlerInfo->factories.find(handlerConfig->type.value()); + if (factoryIter == handlerInfo->factories.end()) { + throw std::invalid_argument(to( + "unknown log handler type \"", handlerConfig->type.value(), "\"")); + } + // Create the new log handler const auto& factory = factoryIter->second; std::shared_ptr handler; try { if (oldHandler) { - handler = factory->updateHandler(oldHandler, entry.second.options); + handler = factory->updateHandler(oldHandler, handlerConfig->options); if (handler != oldHandler) { oldToNewHandlerMap->emplace(oldHandler, handler); } } else { - handler = factory->createHandler(entry.second.options); + handler = factory->createHandler(handlerConfig->options); } } catch (const std::exception& ex) { // Errors creating or updating the the log handler are generally due to diff --git a/folly/experimental/logging/docs/Config.md b/folly/experimental/logging/docs/Config.md index 482a9890..1816f3ad 100644 --- a/folly/experimental/logging/docs/Config.md +++ b/folly/experimental/logging/docs/Config.md @@ -70,7 +70,8 @@ special character like a comma or semicolon use the JSON format instead. ::= ":" | - ::= "=" + ::= "=" ":" + | ":" ::= "," "=" | @@ -113,13 +114,22 @@ for this category to be cleared instead. Each log handler configuration section takes the form - NAME=TYPE,OPTION1=VALUE1,OPTION2=VALUE2 + NAME=TYPE:OPTION1=VALUE1,OPTION2=VALUE2 NAME specifies the log handler name, and TYPE specifies the log handler type. A comma separated list of name=value options may follow the log handler name and type. The option list will be passed to the LogHandlerFactory for the specified handler type. +The log handler type may be omitted to update the settings of an existing log +handler object: + + NAME:OPTION1=VALUE1 + +A log handler with this name must already exist. Options specified in the +configuration will be updated with their new values, and any option names not +mentioned will be left unchanged. + ### Examples @@ -143,13 +153,13 @@ Example log configuration strings: therefore be discarded, even though they are enabled for one of its parent categories. -* `ERROR:stderr, folly=INFO; stderr=stream,stream=stderr` +* `ERROR:stderr, folly=INFO; stderr=stream:stream=stderr` Sets the root log category level to ERROR, and sets its handler list to use the "stderr" handler. Sets the folly log level to INFO. Defines a log handler named "stderr" which writes to stderr. -* `ERROR:x,folly=INFO:y;x=stream,stream=stderr;y=file,path=/tmp/y.log` +* `ERROR:x,folly=INFO:y;x=stream:stream=stderr;y=file:path=/tmp/y.log` Defines two log handlers: "x" which writes to stderr and "y" which writes to the file /tmp/y.log @@ -157,7 +167,7 @@ Example log configuration strings: "x" handler. Sets the log level for the "folly" category to INFO and configures it to use the "y" handler. -* `ERROR:default:x; default=stream,stream=stderr; x=file,path=/tmp/x.log` +* `ERROR:default:x; default=stream:stream=stderr; x=file:path=/tmp/x.log` Defines two log handlers: "default" which writes to stderr and "x" which writes to the file /tmp/x.log @@ -172,13 +182,21 @@ Example log configuration strings: to the empty list. Not specifying handler information at all (no ':') will leave any pre-existing handlers as-is. -* `;default=stream,stream=stdout` +* `;default=stream:stream=stdout` Does not change any log category settings, and defines a "default" handler that writes to stdout. This format is useful to update log handler settings if the "default" handler already exists and is attached to existing log categories. +* `ERROR; stderr:async=true` + + Sets the root log category level to ERR, and sets the "async" property to + true on the "stderr" handler. A log handler named "stderr" must already + exist. Therefore this configuration string is only valid to use with + `LoggerDB::updateConfig()`, and cannot be used with + `LoggerDB::resetConfig()`. + JSON Configuration Syntax ------------------------- @@ -232,9 +250,14 @@ following fields: * `type` - This field is required. It should be a string containing the name of the log - handler type. This type name must correspond to `LogHandlerFactory` type - registered with the `LoggerDB`. + This field should be a string containing the name of the log handler type. + This type name must correspond to `LogHandlerFactory` type registered with + the `LoggerDB`. + + If this field is not present then this configuration will be used to update + an existing log handler. A log handler with this name must already exist. + The values from the `options` field will be merged into the existing log + handler options. * `options` diff --git a/folly/experimental/logging/test/ConfigParserTest.cpp b/folly/experimental/logging/test/ConfigParserTest.cpp index 6eef6bcd..6ef2d1bc 100644 --- a/folly/experimental/logging/test/ConfigParserTest.cpp +++ b/folly/experimental/logging/test/ConfigParserTest.cpp @@ -41,7 +41,7 @@ std::ostream& operator<<(std::ostream& os, const LogCategoryConfig& config) { } std::ostream& operator<<(std::ostream& os, const LogHandlerConfig& config) { - os << config.type; + os << (config.type ? config.type.value() : "[no type]"); bool first = true; for (const auto& opt : config.options) { if (!first) { @@ -106,7 +106,7 @@ TEST(LogConfig, parseBasic) { Pair("", LogCategoryConfig{LogLevel::ERR, true, {}}))); EXPECT_THAT(config.getHandlerConfigs(), UnorderedElementsAre()); - config = parseLogConfig(" ERR:stderr; stderr=file,stream=stderr "); + config = parseLogConfig(" ERR:stderr; stderr=stream:stream=stderr "); EXPECT_THAT( config.getCategoryConfigs(), UnorderedElementsAre( @@ -114,12 +114,12 @@ TEST(LogConfig, parseBasic) { EXPECT_THAT( config.getHandlerConfigs(), UnorderedElementsAre( - Pair("stderr", LogHandlerConfig{"file", {{"stream", "stderr"}}}))); + Pair("stderr", LogHandlerConfig{"stream", {{"stream", "stderr"}}}))); config = parseLogConfig( "ERR:myfile:custom, folly=DBG2, folly.io:=WARN:other;" - "myfile=file,path=/tmp/x.log; " - "custom=custom,foo=bar,hello=world,a = b = c; " + "myfile=file:path=/tmp/x.log; " + "custom=custom:foo=bar,hello=world,a = b = c; " "other=custom2"); EXPECT_THAT( config.getCategoryConfigs(), @@ -141,8 +141,36 @@ TEST(LogConfig, parseBasic) { {{"foo", "bar"}, {"hello", "world"}, {"a", "b = c"}}}), Pair("other", LogHandlerConfig{"custom2"}))); + // Test updating existing handler configs, with no handler type + config = parseLogConfig("ERR;foo"); + EXPECT_THAT( + config.getCategoryConfigs(), + UnorderedElementsAre(Pair("", LogCategoryConfig{LogLevel::ERR, true}))); + EXPECT_THAT( + config.getHandlerConfigs(), + UnorderedElementsAre(Pair("foo", LogHandlerConfig{}))); + + config = parseLogConfig("ERR;foo:a=b,c=d"); + EXPECT_THAT( + config.getCategoryConfigs(), + UnorderedElementsAre(Pair("", LogCategoryConfig{LogLevel::ERR, true}))); + EXPECT_THAT( + config.getHandlerConfigs(), + UnorderedElementsAre(Pair( + "foo", LogHandlerConfig{folly::none, {{"a", "b"}, {"c", "d"}}}))); + + config = parseLogConfig("ERR;test=file:path=/tmp/test.log;foo:a=b,c=d"); + EXPECT_THAT( + config.getCategoryConfigs(), + UnorderedElementsAre(Pair("", LogCategoryConfig{LogLevel::ERR, true}))); + EXPECT_THAT( + config.getHandlerConfigs(), + UnorderedElementsAre( + Pair("foo", LogHandlerConfig{folly::none, {{"a", "b"}, {"c", "d"}}}), + Pair("test", LogHandlerConfig{"file", {{"path", "/tmp/test.log"}}}))); + // Log handler changes with no category changes - config = parseLogConfig("; myhandler=custom,foo=bar"); + config = parseLogConfig("; myhandler=custom:foo=bar"); EXPECT_THAT(config.getCategoryConfigs(), UnorderedElementsAre()); EXPECT_THAT( config.getHandlerConfigs(), @@ -219,13 +247,7 @@ TEST(LogConfig, parseBasicErrors) { EXPECT_THROW_RE( parseLogConfig("ERR;"), LogConfigParseError, - "error parsing log handler configuration \"\": " - "expected data in the form NAME=TYPE"); - EXPECT_THROW_RE( - parseLogConfig("ERR;foo"), - LogConfigParseError, - "error parsing log handler configuration \"foo\": " - "expected data in the form NAME=TYPE"); + "error parsing log handler configuration: empty log handler name"); EXPECT_THROW_RE( parseLogConfig("ERR;foo="), LogConfigParseError, @@ -238,8 +260,18 @@ TEST(LogConfig, parseBasicErrors) { EXPECT_THROW_RE( parseLogConfig("ERR;handler1=file;"), LogConfigParseError, - "error parsing log handler configuration \"\": " - "expected data in the form NAME=TYPE"); + "error parsing log handler configuration: empty log handler name"); + EXPECT_THROW_RE( + parseLogConfig("ERR;test=file,path=/tmp/test.log;foo:a=b,c=d"), + LogConfigParseError, + "error parsing configuration for log handler \"test\": " + "invalid type \"file,path=/tmp/test.log\": type name cannot contain " + "a comma when using the basic config format"); + EXPECT_THROW_RE( + parseLogConfig("ERR;test,path=/tmp/test.log;foo:a=b,c=d"), + LogConfigParseError, + "error parsing configuration for log handler \"test,path\": " + "name cannot contain a comma when using the basic config format"); } TEST(LogConfig, parseJson) { @@ -545,7 +577,7 @@ TEST(LogConfig, toJson) { config = parseLogConfig( "ERROR:h1,foo.bar:=FATAL,folly=INFO:; " - "h1=custom,foo=bar"); + "h1=custom:foo=bar"); expectedJson = folly::parseJson(R"JSON({ "categories" : { "" : { @@ -584,7 +616,7 @@ TEST(LogConfig, mergeConfigs) { EXPECT_THAT(config.getHandlerConfigs(), UnorderedElementsAre()); config = - parseLogConfig("WARN:default; default=custom,opt1=value1,opt2=value2"); + parseLogConfig("WARN:default; default=custom:opt1=value1,opt2=value2"); config.update(parseLogConfig("folly.io=DBG2,foo=INFO")); EXPECT_THAT( config.getCategoryConfigs(), @@ -602,7 +634,7 @@ TEST(LogConfig, mergeConfigs) { // Updating the root category's log level without specifying // handlers should leave its current handler list intact config = - parseLogConfig("WARN:default; default=custom,opt1=value1,opt2=value2"); + parseLogConfig("WARN:default; default=custom:opt1=value1,opt2=value2"); config.update(parseLogConfig("ERR")); EXPECT_THAT( config.getCategoryConfigs(), @@ -616,7 +648,7 @@ TEST(LogConfig, mergeConfigs) { "custom", {{"opt1", "value1"}, {"opt2", "value2"}})))); config = - parseLogConfig("WARN:default; default=custom,opt1=value1,opt2=value2"); + parseLogConfig("WARN:default; default=custom:opt1=value1,opt2=value2"); config.update(parseLogConfig(".:=ERR")); EXPECT_THAT( config.getCategoryConfigs(), @@ -631,7 +663,7 @@ TEST(LogConfig, mergeConfigs) { // Test clearing the root category's log handlers config = - parseLogConfig("WARN:default; default=custom,opt1=value1,opt2=value2"); + parseLogConfig("WARN:default; default=custom:opt1=value1,opt2=value2"); config.update(parseLogConfig("FATAL:")); EXPECT_THAT( config.getCategoryConfigs(), @@ -643,4 +675,28 @@ TEST(LogConfig, mergeConfigs) { "default", LogHandlerConfig( "custom", {{"opt1", "value1"}, {"opt2", "value2"}})))); + + // Test updating the settings on a log handler + config = + parseLogConfig("WARN:default; default=stream:stream=stderr,async=false"); + config.update(parseLogConfig("INFO; default:async=true")); + EXPECT_THAT( + config.getCategoryConfigs(), + UnorderedElementsAre( + Pair("", LogCategoryConfig{LogLevel::INFO, true, {"default"}}))); + EXPECT_THAT( + config.getHandlerConfigs(), + UnorderedElementsAre(Pair( + "default", + LogHandlerConfig( + "stream", {{"stream", "stderr"}, {"async", "true"}})))); + + // Updating the settings for a non-existent log handler should fail + config = + parseLogConfig("WARN:default; default=stream:stream=stderr,async=false"); + EXPECT_THROW_RE( + config.update(parseLogConfig("INFO; other:async=true")), + std::invalid_argument, + "cannot update configuration for " + "unknown log handler \"other\""); } diff --git a/folly/experimental/logging/test/ConfigUpdateTest.cpp b/folly/experimental/logging/test/ConfigUpdateTest.cpp index d0743ced..ad48453d 100644 --- a/folly/experimental/logging/test/ConfigUpdateTest.cpp +++ b/folly/experimental/logging/test/ConfigUpdateTest.cpp @@ -64,7 +64,7 @@ std::ostream& operator<<( } auto config = configHandler->getConfig(); - os << "ConfigHandler(" << config.type; + os << "ConfigHandler(" << (config.type ? config.type.value() : "[no type]"); for (const auto& entry : config.options) { os << ", " << entry.first << "=" << entry.second; } @@ -125,14 +125,14 @@ TEST(ConfigUpdate, updateConfig) { EXPECT_EQ(parseLogConfig(".:=ERROR:"), db.getConfig()); // Apply an update - db.updateConfig(parseLogConfig("INFO:stderr; stderr=handlerA,stream=stderr")); + db.updateConfig(parseLogConfig("INFO:stderr; stderr=handlerA:stream=stderr")); EXPECT_EQ(LogLevel::INFO, db.getCategory("")->getLevel()); EXPECT_THAT( db.getCategory("")->getHandlers(), UnorderedElementsAre( MatchLogHandler("handlerA", {{"stream", "stderr"}}))); EXPECT_EQ( - parseLogConfig(".:=INFO:stderr; stderr=handlerA,stream=stderr"), + parseLogConfig(".:=INFO:stderr; stderr=handlerA:stream=stderr"), db.getConfig()); // Update the log level for category "foo" @@ -146,14 +146,14 @@ TEST(ConfigUpdate, updateConfig) { EXPECT_EQ(1, db.getCategory("")->getHandlers().size()); EXPECT_EQ( parseLogConfig( - ".:=INFO:stderr, foo:=DBG2:; stderr=handlerA,stream=stderr"), + ".:=INFO:stderr, foo:=DBG2:; stderr=handlerA:stream=stderr"), db.getConfig()); // Add 2 log handlers to the "bar" log category. db.updateConfig( parseLogConfig("bar=ERROR:new:h2; " - "new=handlerB,key=value; " - "h2=handlerA,foo=bar")); + "new=handlerB:key=value; " + "h2=handlerA:foo=bar")); EXPECT_EQ(LogLevel::INFO, db.getCategory("")->getLevel()); EXPECT_THAT( db.getCategory("")->getHandlers(), @@ -167,15 +167,15 @@ TEST(ConfigUpdate, updateConfig) { MatchLogHandler("handlerA", {{"foo", "bar"}}))); EXPECT_EQ( parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=ERROR:new:h2; " - "stderr=handlerA,stream=stderr; " - "new=handlerB,key=value; " - "h2=handlerA,foo=bar"), + "stderr=handlerA: stream=stderr; " + "new=handlerB: key=value; " + "h2=handlerA: foo=bar"), db.getConfig()); // Updating the "new" log handler settings should automatically update // the settings we see on the "bar" category, even if we don't explicitly // list "bar" in the config update - db.updateConfig(parseLogConfig("; new=handlerB,newkey=newvalue")); + db.updateConfig(parseLogConfig("; new=handlerB:newkey=newvalue")); EXPECT_EQ(LogLevel::INFO, db.getCategory("")->getLevel()); EXPECT_THAT( db.getCategory("")->getHandlers(), @@ -189,9 +189,9 @@ TEST(ConfigUpdate, updateConfig) { MatchLogHandler("handlerA", {{"foo", "bar"}}))); EXPECT_EQ( parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=ERROR:new:h2; " - "stderr=handlerA,stream=stderr; " - "new=handlerB,newkey=newvalue; " - "h2=handlerA,foo=bar"), + "stderr=handlerA: stream=stderr; " + "new=handlerB: newkey=newvalue; " + "h2=handlerA: foo=bar"), db.getConfig()); // Updating the level settings for the "bar" handler should leave its @@ -210,16 +210,16 @@ TEST(ConfigUpdate, updateConfig) { MatchLogHandler("handlerA", {{"foo", "bar"}}))); EXPECT_EQ( parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=WARN:new:h2; " - "stderr=handlerA,stream=stderr; " - "new=handlerB,newkey=newvalue; " - "h2=handlerA,foo=bar"), + "stderr=handlerA: stream=stderr; " + "new=handlerB: newkey=newvalue; " + "h2=handlerA: foo=bar"), db.getConfig()); // Update the options for the h2 handler in place, and also add it to the // "test.foo" category. The changes should also be reflected on the "bar" // category. db.updateConfig( - parseLogConfig("test.foo=INFO:h2; h2=handlerA,reuse_handler=1,foo=xyz")); + parseLogConfig("test.foo=INFO:h2; h2=handlerA:reuse_handler=1,foo=xyz")); EXPECT_EQ(LogLevel::INFO, db.getCategory("")->getLevel()); EXPECT_THAT( db.getCategory("")->getHandlers(), @@ -240,11 +240,71 @@ TEST(ConfigUpdate, updateConfig) { EXPECT_EQ( parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=WARN:new:h2, " "test.foo=INFO:h2; " - "stderr=handlerA,stream=stderr; " - "new=handlerB,newkey=newvalue; " - "h2=handlerA,reuse_handler=1,foo=xyz"), + "stderr=handlerA: stream=stderr; " + "new=handlerB: newkey=newvalue; " + "h2=handlerA: reuse_handler=1,foo=xyz"), db.getConfig()); + // Update the options for the h2 handler using partial options + db.updateConfig(parseLogConfig("; h2:abc=def")); + EXPECT_EQ(LogLevel::INFO, db.getCategory("")->getLevel()); + EXPECT_THAT( + db.getCategory("")->getHandlers(), + UnorderedElementsAre( + MatchLogHandler("handlerA", {{"stream", "stderr"}}))); + EXPECT_EQ(LogLevel::WARN, db.getCategory("bar")->getLevel()); + EXPECT_THAT( + db.getCategory("bar")->getHandlers(), + UnorderedElementsAre( + MatchLogHandler("handlerB", {{"newkey", "newvalue"}}), + MatchLogHandler( + "handlerA", + {{"abc", "def"}, {"foo", "xyz"}, {"reuse_handler", "1"}}))); + EXPECT_EQ(LogLevel::INFO, db.getCategory("test.foo")->getLevel()); + EXPECT_THAT( + db.getCategory("test.foo")->getHandlers(), + UnorderedElementsAre(MatchLogHandler( + "handlerA", + {{"abc", "def"}, {"foo", "xyz"}, {"reuse_handler", "1"}}))); + EXPECT_EQ( + parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=WARN:new:h2, " + "test.foo=INFO:h2; " + "stderr=handlerA: stream=stderr; " + "new=handlerB: newkey=newvalue; " + "h2=handlerA: reuse_handler=1,abc=def,foo=xyz"), + db.getConfig()); + + // Update the options for the "new" handler using partial options + db.updateConfig(parseLogConfig("; new:opt1=value1")); + EXPECT_EQ(LogLevel::WARN, db.getCategory("bar")->getLevel()); + EXPECT_THAT( + db.getCategory("bar")->getHandlers(), + UnorderedElementsAre( + MatchLogHandler( + "handlerB", {{"opt1", "value1"}, {"newkey", "newvalue"}}), + MatchLogHandler( + "handlerA", + {{"abc", "def"}, {"foo", "xyz"}, {"reuse_handler", "1"}}))); + EXPECT_EQ(LogLevel::INFO, db.getCategory("test.foo")->getLevel()); + EXPECT_THAT( + db.getCategory("test.foo")->getHandlers(), + UnorderedElementsAre(MatchLogHandler( + "handlerA", + {{"abc", "def"}, {"foo", "xyz"}, {"reuse_handler", "1"}}))); + EXPECT_EQ( + parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=WARN:new:h2, " + "test.foo=INFO:h2; " + "stderr=handlerA: stream=stderr; " + "new=handlerB: newkey=newvalue,opt1=value1; " + "h2=handlerA: reuse_handler=1,abc=def,foo=xyz"), + db.getConfig()); + + // Supplying partial options for a non-existent log handler should fail + EXPECT_THROW_RE( + db.updateConfig(parseLogConfig("; no_such_handler:foo=bar")), + std::invalid_argument, + "cannot update unknown log handler \"no_such_handler\""); + // Explicitly clear the handlers for the "bar" category // This should remove the "new" handler from the LoggerDB since bar was the // only category referring to it. @@ -259,17 +319,17 @@ TEST(ConfigUpdate, updateConfig) { EXPECT_EQ( parseLogConfig(".:=INFO:stderr, foo:=DBG2:, bar=WARN:, " "test.foo=INFO:h2; " - "stderr=handlerA,stream=stderr; " - "h2=handlerA,reuse_handler=1,foo=xyz"), + "stderr=handlerA: stream=stderr; " + "h2=handlerA: reuse_handler=1,abc=def,foo=xyz"), db.getConfig()); // Now test resetConfig() db.resetConfig( parseLogConfig("bar=INFO:h2, test.abc=DBG3; " - "h2=handlerB,abc=xyz")); + "h2=handlerB: abc=xyz")); EXPECT_EQ( parseLogConfig(".:=ERR:, bar=INFO:h2, test.abc=DBG3:; " - "h2=handlerB,abc=xyz"), + "h2=handlerB: abc=xyz"), db.getConfig()); } @@ -289,7 +349,7 @@ TEST(ConfigUpdate, getConfigAnonymousHandlers) { db.getCategory("x.y.z")->addHandler(handlerFoo); EXPECT_EQ( parseLogConfig(".:=ERR:, x.y.z=DBG2:anonymousHandler1; " - "anonymousHandler1=foo,abc=xyz"), + "anonymousHandler1=foo:abc=xyz"), db.getConfig()); // If we attach the same handler to another category it should still only be @@ -300,20 +360,20 @@ TEST(ConfigUpdate, getConfigAnonymousHandlers) { parseLogConfig(".:=ERR:, " "x.y.z=DBG2:anonymousHandler1, " "test.category=DBG1:anonymousHandler1; " - "anonymousHandler1=foo,abc=xyz"), + "anonymousHandler1=foo:abc=xyz"), db.getConfig()); // If we use updateConfig() to explicitly define a handler named // "anonymousHandler1", the unnamed handler will be reported as // "anonymousHandler2" instead now. db.updateConfig(parseLogConfig( - "a.b.c=INFO:anonymousHandler1; anonymousHandler1=handlerA,key=value")); + "a.b.c=INFO:anonymousHandler1; anonymousHandler1=handlerA:key=value")); EXPECT_EQ( parseLogConfig(".:=ERR:, " "a.b.c=INFO:anonymousHandler1, " "x.y.z=DBG2:anonymousHandler2, " "test.category=DBG1:anonymousHandler2; " - "anonymousHandler1=handlerA,key=value; " - "anonymousHandler2=foo,abc=xyz"), + "anonymousHandler1=handlerA: key=value; " + "anonymousHandler2=foo: abc=xyz"), db.getConfig()); } diff --git a/folly/experimental/logging/test/fatal_test.py b/folly/experimental/logging/test/fatal_test.py index fa7a2a9b..bca4fbd6 100644 --- a/folly/experimental/logging/test/fatal_test.py +++ b/folly/experimental/logging/test/fatal_test.py @@ -69,12 +69,12 @@ class FatalTests(unittest.TestCase): subprocess.check_output([self.helper, '--crash=no']) def test_async(self): - handler_setings = 'default=stream,stream=stderr,async=true' + handler_setings = 'default=stream:stream=stderr,async=true' err = self.run_helper('--logging=;' + handler_setings) self.assertRegex(err, self.get_crash_regex()) def test_immediate(self): - handler_setings = 'default=stream,stream=stderr,async=false' + handler_setings = 'default=stream:stream=stderr,async=false' err = self.run_helper('--logging=;' + handler_setings) self.assertRegex(err, self.get_crash_regex()) -- 2.34.1