Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export/Import contents of LogContext #773

Closed
IanYates opened this Issue Jun 14, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@IanYates
Copy link
Contributor

IanYates commented Jun 14, 2016

Hi,

Following on from #529 , I've got my own wrapped up timings methods where, if the Action or Func takes longer than expected I log out messages repeatedly to Seq via Serilog. It's helpful to see how much activity is spent waiting on some shared mutex and I can track back to the source (although my Seq filtering skills in 3.x are not well exercised yet).

These timing messages are done via a timer - on its first trigger it'll log a warning and subsequent triggers it bumps things up to error. This is different from simply logging start & finish + duration since I'd like insight into some process that's longer-running than expected whilst it's still running. (example: it came in handy when I accidentally had an async void Action<T>, rather than an async task Func<T,Task>, lambda leading to some weird deadlock behaviour).

Unfortunately this all happens on a background timer thread so the contents of LogContext that I'd like to include is effectively lost.

A suggested LogContext API in from #529 was

var parent = LogContext.Export();

Task.Run(() =>
{
   LogContext.Import(parent);

   Log.Information("Here 'tis");
});

Is this still a good idea? Would a PR along these lines be accepted, or is something like it already possible today and I just haven't noticed?

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Jun 14, 2016

Hi Ian - interesting scenario.

Export/Import seems like it would cover this, but since LogContext might already be active in the destination task (not in your example, but generally), the import method would need to be more like a "push", adding to the destination context for a time until it is popped off again.

I'm not 100% certain it'd all fall out nicely, but for the sake of discussion what do you make of:

var parent = LogContext.Clone();

Task.Run(() =>
{
   using (LogContext.Push(parent))
   {
      Log.Information("Here 'tis");
   }
});

In this case, Clone() would return ILogEventEnricher, and Push(ILogEventEnricher) would be a new method eventually replacing the awkwardly-named PushProperties(). Including Push(ILogEventEnricher) and Push(IEnumerable<ILogEventEnricher>) or similar in the PR while [Obsolete]ing PushProperties() would be a nice tidy-up.

@IanYates

This comment has been minimized.

Copy link
Contributor Author

IanYates commented Jun 15, 2016

Sounds like a smart approach.

I can go ahead and start a PR on that basis (unless you've already done something on it?). I'd be happy to make it an extension except that it really needs to be in core given it's affecting LogContext.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Jun 15, 2016

I'm not doing anything at this point myself, it sounds well worth writing up as a PR. Keen to get some wider input. Cheers!

@IanYates

This comment has been minimized.

Copy link
Contributor Author

IanYates commented Jun 15, 2016

No worries - I'll get cracking on some internal-use code, try it out and then send it as a PR. I know sometimes you churn things out quickly so I didn't want to do something if you were mulling a gist. I'll come back with questions in this thread if I have any. Thanks for the advice 👍

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Feb 14, 2017

@IanYates did you end up with a feel for how tricky this one might be (or might not be?) Could be a good one to put "up-for-grabs" if it's not underway. Any thoughts?

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented May 28, 2017

Done in #974 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.