Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1004 closed task (complete)

add logging option for 'every program'

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20110628
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

With #736 merged, you can create a logger for any module (i.e. b10-resolver, b10-auth, b10-xfrin, even though not all of these may actually use it yet), but there is no way to just specify a configuration for everything.

The proposal is to make the logging initialization code (in ccsession, the code that reads the configuration and calls the logger-manager) accept "*" as a name, meaning 'replace by whatever program is reading this'.

Subtickets

Change History (15)

comment:1 Changed 9 years ago by jelte

  • Component changed from Unclassified to logging
  • Sub-Project changed from DNS to Core

comment:2 Changed 9 years ago by stephen

  • Milestone changed from New Tasks to Sprint-20110628

comment:3 Changed 9 years ago by jreed

  • Type changed from defect to task

comment:4 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Added a very simply check for * or *., and replaces the * with the current root logger.

There's no tests for this, unfortunately, we need #1047 first

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Hello

With the missing test, should we complete #1047 before merging this or should we merge right away and postpone it? I'm not sure (I know I'm not so strict about writing tests myself, so maybe we should ask someone more strict or bring it up on jabber).

Anyway, looking at the code, if there's an asterisk and a „real“ logger name, I guess which one will be used depends on the order? I'd expect '*' to be used only if the real logger name is not used anywhere (it makes sense to say „log everything as INFO, but xfrout as DEBUG“, the ordering is error prone).

comment:8 in reply to: ↑ 7 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

With the missing test, should we complete #1047 before merging this or should we merge right away and postpone it? I'm not sure (I know I'm not so strict about writing tests myself, so maybe we should ask someone more strict or bring it up on jabber).

I'm not sure, I could personally very much use this feature, and #1047 is not even in this sprint. However, with this last addition (see below), I also think this needs good tests, so perhaps it's prudent to postpone finalization of this one...

Anyway, looking at the code, if there's an asterisk and a „real“ logger name, I guess which one will be used depends on the order? I'd expect '*' to be used only if the real logger name is not used anywhere (it makes sense to say „log everything as INFO, but xfrout as DEBUG“, the ordering is error prone).

Yeah you're totally right. So i've added a new method (and replaced the old *-replacing-code), that makes it ignore all configs for 'unrelated' loggers (i.e. loggers with different root names, which wouldn't do anything anyway), and for '*-loggers' if there is also an explicit one.

Some examples are inline in comments.

One possibly contested one is if you have "*.cache", and "b10-resolver". Right now it will do both ("*.cache" expands to "b10-resolver.cache" which is more specific that "b10-resolver"), which seems to me the right way, but people might disagree.

comment:9 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

While I believe this will work and will be harmless, I'd like to point out this is not completely correct. For one, it will create odd loggers when we have a logger named "*xxx". Another thing is, if there's a logger "b10-stats-http" and current module is "b10-stats", it will not ignore it.

It might be better to use std::set than std::vector.

Anyway, this method can be tested without #1047, because it produces only more JSON.

And I agree with the last paragraph, I find it logic this way.

comment:10 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Ohyeah, I moved the function out of the global namespace so it can be tested.

Added those tests, fixed your two issues (and another one, it did not actually replace the * in the name anymore).

Also added error reporting to the logging conf plugin in cfgmgr, and discovered it had no tests either, so i added those as well (and fixed a couple of smaller issues there too)

comment:11 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Good there are tests for the plugin now as well.

I see only two minor things:

  • The condition looks really complicated, the negation of compound expression makes it hard to read:
    if (cur_name.length() > 0 && cur_name[0] == '*' &&
        !(cur_name.length() > 1 && cur_name[1] != '.')) {
    
    Could it be made as positive with ||?
  • The way the JSON is copied looks less elegant than necessary. The thing I don't like about it is it creates a deep copy of the JSON, while shallow one could be enough. Since the toplevel element must be a MapElement?, we could typecast it to it and using a default copy constructor of MapElement?, create new one and use that one to replace the one "name" subelement. I'm not sure this is more elegant, and the data will not be too deep anyway, so I'll leave it up to you.

Thanks

comment:12 in reply to: ↑ 11 Changed 9 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

Good there are tests for the plugin now as well.

I see only two minor things:

  • The condition looks really complicated, the negation of compound expression makes it hard to read:
    if (cur_name.length() > 0 && cur_name[0] == '*' &&
        !(cur_name.length() > 1 && cur_name[1] != '.')) {
    
    Could it be made as positive with ||?

sure

  • The way the JSON is copied looks less elegant than necessary. The thing I don't like about it is it creates a deep copy of the JSON, while shallow one could be enough. Since the toplevel element must be a MapElement?, we could typecast it to it and using a default copy constructor of MapElement?, create new one and use that one to replace the one "name" subelement. I'm not sure this is more elegant, and the data will not be too deep anyway, so I'll leave it up to you.

actually, i originally chose to use the json repr because i wanted a deep copy, but indeed with an appropriate explanation, a shallow copy is enough. We don't even need a cast for that (creating a new mapelement and using setvalue works just as well, though i did have to fix a missing const in data.h)

comment:13 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

Thanks, looks good.

Considering the only untested code now is the one already untested in master, I think this can be merged.

comment:14 Changed 9 years ago by jelte

  • Resolution set to complete
  • Status changed from reviewing to closed

Done, thanks!

Closing ticket.

comment:15 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3
Note: See TracTickets for help on using tickets.