Opened 3 years ago

Last modified 20 months ago

#20430 assigned enhancement

Define common log levels

Reported by: iwakeh Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords: metrics-2018
Cc: Actual Points:
Parent ID: #20540 Points:
Reviewer: Sponsor:

Description

Logging for metrics-lib was introduced a while ago, what's still missing is defining appropriate log levels.

This ticket should be for discussion of a log-level scheme and also collecting examples for useful and less useful log level choices.

Child Tickets

Change History (17)

comment:1 Changed 3 years ago by iwakeh

The following should rather be info level, b/c this happens once in a while and doesn't constitute an error. (and errors are mailed by the some logging frameworks.)

2016-10-21 17:35:20,640 CollecTor-Scheduled-Thread-7 ERROR org.torproject.descriptor.index.DescriptorIndexCollector Cannot fetch remote file: 2016-10-18-17-05-00-server-descriptors
java.io.FileNotFoundException: https://collector.torproject.org/recent/relay-descriptors/server-descriptors/2016-10-18-17-05-00-server-descriptors

comment:2 in reply to:  1 Changed 3 years ago by karsten

Replying to iwakeh:

The following should rather be info level, b/c this happens once in a while and doesn't constitute an error. (and errors are mailed by the some logging frameworks.)

2016-10-21 17:35:20,640 CollecTor-Scheduled-Thread-7 ERROR org.torproject.descriptor.index.DescriptorIndexCollector Cannot fetch remote file: 2016-10-18-17-05-00-server-descriptors
java.io.FileNotFoundException: https://collector.torproject.org/recent/relay-descriptors/server-descriptors/2016-10-18-17-05-00-server-descriptors

Good point. I agree that this log message shouldn't be an error. Though I'm not sure if info level is too quiet. After all, if we cannot fetch a single file referenced from an index.json, that would be worth a warning. Should I change this to a warning for now? Or did you only want to bring this up as example and change levels later?

comment:3 Changed 3 years ago by iwakeh

It could be warning. I avoided the message by slightly changing the schedule. Thus, the clean-up is not running anymore on the main CollecTor while the mirror syncs from there.

I was considering info level because it is a normal thing to happen when syncing from an instance that is cleaning its recent folder.

This ticket could also be input for the level definition, that's true.

comment:4 Changed 3 years ago by karsten

Ideally, we'll want to keep the index.json of a CollecTor instance in sync with the contents it serves. And that will become better as soon as we turn the index.json update into a method that gets called at the end of each module. Another thing that we could do is omit files from index.json when they're older than 72 hours but only delete them when they get older than 73 hours.

Okay, I'm leaving this unchanged for the moment, because we're not going to release a new metrics-lib version in the next days anyway, and maybe we'll have a better idea whether this should be an informational message or a warning as we consider more cases.

comment:5 Changed 3 years ago by iwakeh

Parent ID: #20540

Ticket for applying the general log-level definitions from parent ticket.

comment:6 Changed 3 years ago by karsten

Looks like we already turned the error above into a warning as part of #20540 where we improved logging of the index package.

What remains to be done here, and which part of that is blocking the 1.6.0 release?

comment:7 Changed 3 years ago by iwakeh

I see this ticket as reminder to apply the log-level scheme we derived in #20540 to all of metrics-lib.

That is these steps:

  • check current logging and turn any leftover printStackTrace into logging or throw an Exception
  • check Exceptions thrown, if the reason rather should be logged and not thrown
  • maybe, identify issues worth logging

and the hidden task of verifying the description of the logging scheme, i.e., if it makes sense and is a usable guideline for metrics-lib.

In total rather a task of spending a fixed amount of time to verify these things, not a blocker to the release.

comment:8 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.6.0metrics-lib 2.0.0

Looking at #16225 this ticket should be in the same release as #16225.
Thus, moving it to milestone 2.0.0 (and eventually back to 1.7.0 once that is defined).

comment:9 Changed 3 years ago by iwakeh

Milestone: metrics-lib 2.0.0metrics-lib 1.7.0

comment:10 Changed 2 years ago by iwakeh

Milestone: metrics-lib 1.7.0metrics-lib 1.8.0

comment:11 Changed 2 years ago by karsten

Milestone: metrics-lib 1.8.0

Removing from milestones for now. It's important to do this, but it's not as time-critical as some other tasks.

comment:12 Changed 2 years ago by karsten

When we get back to this ticket, let's consider turning log messsage "Serving implementation {} for {}." into a debug message. See also the related discussion on #22476.

comment:13 Changed 2 years ago by karsten

Owner: changed from karsten to metrics-team
Status: newassigned

Handing over to metrics-team, because I'm not currently working on this.

comment:14 Changed 2 years ago by karsten

Summary: log-level definition for metrics-libDefine common log levels

Tweak summary.

comment:15 Changed 23 months ago by karsten

Keywords: metrics-2018 added

comment:16 Changed 23 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:17 Changed 20 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

Note: See TracTickets for help on using tickets.