Project

General

Profile

Feature #3918

Advanced filtering of the logging modules

Added by Alex Afanasyev over 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Utils
Target version:
Start date:
06/07/2017
Due date:
% Done:

100%

Estimated time:

Description

Right now, the logging supports filtering by the exact module name and log priority. It would be good to allow designating a "group" (e.g., by a prefix, but some other definition of the group could be ok too) and enable logging for that group without explicitly enumerating all modules.

History

#1 Updated by Damian Coomes almost 2 years ago

  • Status changed from New to In Progress
  • Assignee set to Damian Coomes
  • Start date set to 06/07/2017
  • % Done changed from 0 to 70

#2 Updated by Damian Coomes almost 2 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 70 to 90

#3 Updated by Junxiao Shi almost 2 years ago

  • Status changed from Code review to Feedback

https://gerrit.named-data.net/3934 patchset2 introduces several new functions on Logging class. It shouldn't be so complicated at API level.

The convention has been: declare logger names using dot notation. For example: "ndn.Controller", "ndn.Face", "nlsr.Adjacency", "ndnping.Client".
From there, we can just define a string syntax for NDN_LOG environ and Logging::setLevel. For example: "*=INFO:ndn.*=NONE:nlsr.*=DEBUG:ndn.Dispatcher=ERROR".

#4 Updated by Junxiao Shi almost 2 years ago

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.

#5 Updated by Davide Pesavento almost 2 years ago

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.

#6 Updated by Junxiao Shi almost 2 years ago

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.

#7 Updated by Davide Pesavento almost 2 years ago

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.

#8 Updated by Alex Afanasyev over 1 year ago

  • Status changed from Feedback to New
  • Target version set to v0.6

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.

#9 Updated by Davide Pesavento over 1 year ago

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/*

#10 Updated by Alex Afanasyev over 1 year ago

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

#11 Updated by Damian Coomes over 1 year ago

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

#12 Updated by Alex Afanasyev over 1 year ago

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

#13 Updated by Damian Coomes over 1 year ago

  • Status changed from New to Feedback

#14 Updated by Davide Pesavento over 1 year ago

  • Category changed from Base to Utils
  • Status changed from Feedback to Closed
  • % Done changed from 90 to 100

A man page has been added.

Also available in: Atom PDF