NFD » ndn-cxx

https://gerrit.named-data.net/3934 patchset6 uses two containers to store the enabled levels:

• std::unordered_map<std::string, LogLevel> m_enabledLevel stores settings for specific modules, and for "*" which applies to all modules.
• std::unordered_map<std::string, std::string> m_filterMap stores settings for prefix filtering. For example, "fm.*" applies to any log module starting with "fm.".

This separation is unnecessary. Only one container should be used, which stores all the settings.

• Log module name must not contain "*" and must not end with ".". This prevents ambiguity.
• m_enabledLevel stores settings for specific modules using keys like "ndn.Face", settings for module prefixes using keys like "ndn." (note that there's no "*" at the end), and the default setting using an empty string as the key.
• When a new log module is created, a longest prefix match is performed in m_enabledLevel to determine its setting. For example, "ndn.Face" searches "ndn.Face", "ndn.", and "".
• Setting for a specific module is inserted directly to m_enabledLevel, possibly replacing an existing entry.
• Inserting setting for a module prefix or the default setting causes the deletion of more specific settings. For example, inserting "ndn." erases "ndn.Face", and inserting "" erases "ndn." and "ndn.Face".
• Whenever a setting is updated, the enabled level of affected loggers in m_loggers are updated.

I think having m_filterMap is valuable since m_enabledLevel continues to grow as new loggers are added, but m_filterMap would likely remain relatively small. Therefore, it would be faster to traverse. However, moving "" into m_filterMap would cause the fnmatch to match everything with the form ".*"

• m_enabledLevel should not grow as new loggers are added. It only stores settings coming from Logging::setLevel function or environ.
• The container is never traversed in its entirety.
• Determining the enabled level for a new logger is a longest prefix match in m_enabledLevels which takes O(L logM) time where L is number of name components, and M is number of settings.
• During the insertion of setting for a module prefix, it takes O(logM) time to determine the range of more specific settings to be deleted, and O(logN) time to determine the range of affected loggers where N is number of loggers.
• fnmatch should not be used.

Inserting setting for a module prefix or the default setting causes the deletion of more specific settings. For example, inserting "ndn." erases "ndn.Face", and inserting "" erases "ndn." and "ndn.Face".

I feel it should be the opposite: more specific settings should have priority over less specific (more generic) settings.

fnmatch should not be used.

Why? I think we should prefer using a well-known pattern matching syntax (either glob or regex) rather than inventing our own.

Inserting setting for a module prefix or the default setting causes the deletion of more specific settings. For example, inserting "ndn." erases "ndn.Face", and inserting "" erases "ndn." and "ndn.Face".

I feel it should be the opposite: more specific settings should have priority over less specific (more generic) settings.

In the existing implementation, invoking setLevel("*", lvl) sets all loggers to lvl, regardless of previous setting. This issue is about filtering, and should preserve the existing behavior.

fnmatch should not be used.

Why? I think we should prefer using a well-known pattern matching syntax (either glob or regex) rather than inventing our own.

The syntax is somewhat compatible with fnmatch. The difference is that * can only refer to one or more entire components, and can only appear at last. Not using fnmatch function avoids any kind of enumeration in the implementation.

Junxiao Shi wrote:

In the existing implementation, invoking setLevel("*", lvl) sets all loggers to lvl, regardless of previous setting. This issue is about filtering, and should preserve the existing behavior.

Yeah, but at moment we only have exact matching of components, with the sole exception of "*", so the case above is kind of a corner case. With wildcard matching this becomes much more common.

In any case, if you're talking about API calls (setLevel), I'm probably ok with the semantics you propose. However, for setting the initial log levels via NDN_LOG, I think this behavior might be unintuitive.

The syntax is somewhat compatible with fnmatch. The difference is that * can only refer to one or more entire components, and can only appear at last. Not using fnmatch function avoids any kind of enumeration in the implementation.

Well, either it supports globbing or it doesn't. Being "somewhat compatible" doesn't mean anything. For example, I'd expect to be able to do "nfd.Face.*Ethernet*=TRACE" and similar things. This should be easy to match using the FNM_PATHNAME flag to fnmatch(3). Also, I'm not sure if enumeration is a problem, this is not performance-sensitive code.

I won't insist though, if others think this is not needed.

Alex Afanasyev wrote:

The code is merged, now we need documentation. At the very least, it should be a man page. Not quite sure what should be the name of it, may be ndn-log.

uhm why a man page? Everything else is documented in doxygen or in docs/tutorials/*

manpages also appear on the library's homepage. But, I don't really have a preference.

Do you still want the manpage? If so, what needs to be included, aside from how to use filters?

Any documentation would be good. A small tutorial or manpage. As I said, I don't really have a preference.