log: LogRecord

[seanmonstar]

The logging macros now create a LogRecord, and pass that to the Logger. This will allow custom loggers to change the formatting, and possible filter on more properties of the log record.

DefaultLogger's formatting was taken from Python's default formatting:
LEVEL:from: message

Also included: fmt::Arguments now implement Show, so they can be used to
extend format strings.

@alexcrichton r?

[huonw]

Should this be public?

[seanmonstar]

Should it be? I just wrote so I could impl fmt::Show, in order to write a str or u32 without allocating a ~str.

[huonw]

Seems like external loggers might find it useful too.

Maybe the log level could just be

pub struct LogLevel {
    pub level: u32
}

which implements Show as a string, but still provides easy access to the internal number. (I guess the field could be private too, and offer a retrieval method.)

(In any case, with or without any of these changes, this should be storing a LogLevel, not a u32.)

[seanmonstar]

Oh derp, you're right. I changed everything to LogLevel except there.

I'd been thinking of making Level an enum, but that stops people from using
other levels.
On May 2, 2014 7:44 PM, "Huon Wilson" [email protected] wrote:

In src/liblog/lib.rs:

}

struct DefaultLogger {
handle: LineBufferedWriterio::stdio::StdWriter,
}

+struct LevelStr(u32);

Seems like external loggers might find it useful too.

Maybe the log level could just be

pub struct LogLevel {
pub level: u32}

which implements Show as a string, but still provides easy access to the
internal number. (I guess the field could be private too, and offer a
retrieval method.)

(In any case, with or without any of these changes, this should be storing
a LogLevel, not a u32.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13912/files#r12254268
.

[alexcrichton]

I did some code size analysis before and after these changes. The test file I was using had 500 invocations of debug! with a few format arguments.

Before this change, there is 0x00134c70 bytes of text. After this change, there was 0x0013f070 bytes of text, a 41984 byte increase, 83 extra bytes per callsite.

I made a tweak, causing the code size to increase instead to 0x001365a0, a delta of 6448 bytes, 12 bytes per callsite. It does involve more stuff in the data section, but it's less rarely used, so perhaps it's not all that bad.

Could you change the log function to be:

struct LogLocation {
    file: &'static str,
    line: uint,
    module_path: &'static str,
}

fn log(lvl: LogLevel, loc: &LogLocation, args: &fmt::Arguments) { ... }

The LogLocation structure would be statically generated for each invocation of log!, so there's less runtime overhead generating the structure. The LogLocation structure should be hidden from the documentation, and the log function should probably also have doc(hidden).

[seanmonstar]

I quite dislike splitting it into 3 arguments like that. The gain is that the LogLocation fields only need to be constructed once, at compiled time? Would making those fields on the LogRecord static strings make up much of the difference?

[alexcrichton]

The log function is something that will and should never be invoked manually, so I'm not that worried about the ergonomics of it that much. This is largely a question of performance, and from what I saw it is measurably better to create static data than to pass it all through at runtime.

The interface of the logger won't change, it will still take just a LogRecord, it's just the implementation that will change to efficiently marshal things at the callsite of the logging statement.

[seanmonstar]

Oh I see, log will take those args and create a LogRecord. I was worried
the Logger trait was going to be ugly, by removing LogRecord.

[seanmonstar]

@alexcrichton like that?

[alexcrichton]

I'm not sure I'm entirely convinced of this new LogLevel structure. The inner field is private, so it doesn't allow inspecting the value itself, and if it were public, why wrap it in a structure? If it's just used for formatting, it seems like it should be used locally in the implementation of DefaultLogger

[seanmonstar]

My goals are that in other languages, such as Python, a log record contains the level and the levelname. The level is more useful for filtering, such as if record.level >= log::WARN. And then the levelname is more useful in formatting. However, since you can specify custom levels that don't have a name, you might need to output just the level number.

So, I need:

• easy Ord. Probably against a u32, but maybe it'd be fine to ask people to do if record.level >= LogLevel(20)
• Easy outputting of the name or number. I figured I could implement fmt::Show and fmt::Signed for LogLevel. Show would try to show the name if possible, and Signed would always just show the number.
• No need to have both level and levelname as fields on the LogRecord, since they can be derived when needed.

I also wanted to add fmt::Signed to LogLevel.

[alexcrichton]

Ok, in that case, you may also want to make sure the field is public via pub LogLevel(pub u32)

[seanmonstar]

I'm fixing up the missing pieces of LogLevel right now... as a special representation of a u32, does it make since to also include FromPrimitive and ToPrimitive? I don't fully grasp the use of those traits.

[alexcrichton]

I'd stick with the simpler implementation for now and just implement things like Ord

[alexcrichton]

That looks great, thanks! Just a few things:

• There's a travis failure you may be interested in.
• This is a breaking change with respect to the Logger trait, so can you re-word the commit message in accordance with our breaking changes policy?

[seanmonstar]

Ok, improved LogLevel, adjust commit message for "breaking-changes", and fixed failure in travis.