search

nblumhardt / serilog-timings

Extends Serilog with support for timed operations
Apache License 2.0
218 stars 22 forks source link

Naming choices #60

Closed JochemPalmsens closed 11 months ago

JochemPalmsens commented 11 months ago

Hi. Thanks for your work and library. I would like to share my experience with it today. I'm a senior developer working on a desktop application (net 7 winui3) and a colleague of mine recently wanted to introduce logging into our codebase. He chose SeriLog and added a lot of code over the whole repo (I talked to him about that, asking him to split it up into several PRs, but that's another story). Anyhow, the gigantic PR was there, so I decided to review it as-is and somewhere I found the added lines:

using (Operation.Time("Handling activation"))
{
    await activationService.HandleActivation(activatedEventArgs.Data as IActivatedEventArgs, ActivatedInstanceType.NewInstance);
}

I had no idea what Operation.Time means. So I googled it, which didn't give much information. I had to start-up visual studio, go to the file, and find the references to this package to understand what it is.

I really like the feature, but I don't think the naming is very expressive. From reading the code it's not clear that using (Operation.Time([...])) means "measure the execution duration of the code in the using scope and log it to serilog".

Serilog seems to use the Log.Information-like commands. Can't you use something like Log.ExecutionTimeOf or so?

Just my two cents.

nblumhardt commented 11 months ago

Hi! Thanks for the 2c, it's nice to consider these things :+1: I'm not planning any significant API changes in this package in the foreseeable future, so closing as "not planned", but it's useful feedback for a hypothetical "next time" :-)