-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add activity Ids and Context to all logs #34305
Comments
@davidfowl could you please make this issue as needed and assign it to yourself? |
Concerned by "all logs"; more specifically I have high-throughput applications where I do not want the allocation of a requestId or Activity per request (and others where its useful) |
@benaadams This won't introduce any new allocations. We'll likely also do something like what's in this PR dotnet/extensions#2434 (so you can avoid more allocations). |
@davidfowl I took a look at the PR you mentioned. One thing is not clear to me. Does this mean that for a logger to support activity ids/context, a logger provider will need to implement |
dotnet/aspnetcore#11211 - This was the original PR to put the activity information into the logging scope. The plan now is to do it in the logger implicitly so that the current using System;
using System.Diagnostics;
using Microsoft.Extensions.Logging;
namespace ConsoleApp2
{
class Program
{
static void Main(string[] args)
{
using var loggerFactory = LoggerFactory.Create(builder =>
{
builder.AddConsole(o =>
{
// Print out scope information to the console
o.IncludeScopes = true;
});
});
var logger = loggerFactory.CreateLogger<Program>();
// Logging using a manual scope
using (logger.BeginScope(new { A = 1 }))
{
logger.LogInformation("This is a test");
}
// Today logging with an activity doesn't automatically add it to the scope, this is the change
// we want to make work with this change. It should be opt-out in the logger factory.
var activity1 = new Activity("MyActivity");
activity1.Start();
logger.LogInformation("This is another test");
activity1.Stop();
// Manually adding the activity information to the logging scope. This is what customers have to do today.
var activity2 = new Activity("MyActivity2");
activity2.Start();
using (logger.BeginScope(new { activity2.TraceId, ParentId = activity2.ParentSpanId, activity2.SpanId }))
{
logger.LogInformation("This is another test");
}
activity2.Stop();
}
}
} As for implementations I can think of a few techniques:
We need a way to turn this behavior off as well. Likely an option on the provider or factory depending on where we put this behavior. For 1. I'd imagine the code would look like this: public void ForEachScope<TState>(Action<object, TState> callback, TState state)
{
void Report(Scope current)
{
if (current == null)
{
return;
}
Report(current.Parent);
callback(current.State, state);
}
if (Activity.Current != null)
{
// Unfortunate allocation, can potentially be cached on the activity itself...
callback(new ActivityLogScope(Activity.Current), state);
}
Report(_currentScope.Value);
} ActivityLogScope private class ActivityLogScope : IReadOnlyList<KeyValuePair<string, object>>
{
private readonly Activity _activity;
private string _cachedToString;
public int Count
{
get
{
return 3;
}
}
public KeyValuePair<string, object> this[int index]
{
get
{
if (index == 0)
{
return new KeyValuePair<string, object>("SpanId", _activity.GetSpanId());
}
else if (index == 1)
{
return new KeyValuePair<string, object>("TraceId", _activity.GetTraceId());
}
else if (index == 2)
{
return new KeyValuePair<string, object>("ParentId", _activity.GetParentId());
}
throw new ArgumentOutOfRangeException(nameof(index));
}
}
public ActivityLogScope(Activity activity)
{
_activity = activity;
}
public override string ToString()
{
if (_cachedToString == null)
{
_cachedToString = string.Format(
CultureInfo.InvariantCulture,
"SpanId:{0}, TraceId:{1}, ParentId:{2}",
_activity.GetSpanId(),
_activity.GetTraceId(),
_activity.GetParentId());
}
return _cachedToString;
}
public IEnumerator<KeyValuePair<string, object>> GetEnumerator()
{
for (int i = 0; i < Count; ++i)
{
yield return this[i];
}
}
IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}
} |
cc @pakrym |
@davidfowl thanks for the write-up.
public bool EnableActivityLogging { get; set; } = true; of course, we can discuss the property name more.
|
One more idea we can use to reduce the allocation of the ActivityLogScope object is to use a thread-local object in which we can update the activity object in that instance before passing it to the callback. @davidfowl what you think about this idea? |
I am not sure having another Asynclocal would be better. if this is the case, we can stick the object to the Activity through SetCustomProperty as I suggested earlier. Activity Current is already stored in async local anyway. Also, we'll allocate it only when it is requested. I proposed thread-local which means we'll allocate max one object per thread and only when it is needed. I believe this would be much cheaper. My thinking is according to comparing the number of Activities to number of threads used in our logging scenarios. |
We already have an async local in the scope provider that we use for scopes, we can store a second value in it. |
Oh, I didn't know that :-) thanks for pointing out at that. |
The proposal looks quite good to me. Some minor questions/comments:
Prior arts in OpenCensus Python logging integration: https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-logging#opencensus-logging-integration |
Good question. I included the parent according to aspnet core implementation https://github.com/dotnet/aspnetcore/blob/9d2b9e92c68b61e12f41b33efee57bf561e023c4/src/Hosting/Hosting/src/Internal/ActivityExtensions.cs. @davidfowl may advise if this is needed. This issue will not affect the public API, so we can decide about it without any need to change the proposed APIs.
Should we include TraceState or the TraceFlags by default? or you think wouldn't be useful most of the time? I mean do we really need to have separate options to include/exclude these?
This is something we can talk about and plan for it if it is an important scenario we need to support. I love to get more information about that. |
I think @shirhatti may help too telling about aspnet scenario if need to include ParentId, TraceState or the TraceFlags. |
I don't have an authoritative answer at this moment 😄
Sure! Let's focus on the .NET 5 scope here and take this into a separate discussion. |
No we shouldn't. If this becomes a thing we should have a flags enum instead of a bool that describe what pieces of the activity to include. I don't think we should include trace state or flags by default though. |
Ok, it sounds having flags enum will give us some flexibility. I can try updating the proposal as: [Flags]
public enum ActiviyTrackingOptions
{
None = 0x0000,
SpanId = 0x0001,
TraceId = 0x0002,
ParentId = 0x0004,
TraceState = 0x0008,
TraceFlags = 0x0010
}
public class LoggerFactoryOptions
{
public LoggerFactoryOptions() { }
public ActiviyTrackingOptions ActivityTrackingOptions { get {throw null; } set { throw null; } }
}
public partial class LoggerExternalScopeProvider : IExternalScopeProvider
{
public LoggerExternalScopeProvider(ActiviyTrackingOptions activityTrackingOptions) { }
} @davidfowl what do you think? |
Talked offline with @davidfowl and we decided to include the flags enum with adding the Default value (which will be including SpanId, TraceId, and ParentId). Also, we are going to remove the changes in LoggerExternalScopeProvider and we'll use internal implementation instead. I have updated the proposal according that. I'll work modifying my implementation too. |
Thanks @davidfowl @tarekgh! |
thanks @reyang for your valuable feedback. |
namespace Microsoft.Extensions.Logging
{
[Flags]
public enum ActivityTrackingOptions
{
None = 0x0000,
SpanId = 0x0001,
TraceId = 0x0002,
ParentId = 0x0004,
TraceState = 0x0008,
TraceFlags = 0x0010
}
public class LoggerFactoryOptions
{
public LoggerFactoryOptions();
public ActivityTrackingOptions ActivityTrackingOptions { get; set; }
}
public partial class LoggerFactory
{
public LoggerFactory(IEnumerable<ILoggerProvider> providers,
IOptionsMonitor<LoggerFilterOptions> filterOption,
IOptions<LoggerFactoryOptions> options = null);
}
public static partial class LoggingBuilderExtensions
{
public static ILoggingBuilder Configure(this ILoggingBuilder builder,
Action<LoggerFactoryOptions> action);
}
} |
This issue tracking the work as the part of the issue #31372
Rationale and Use Cases
Currently, there is no automatic way to log the tracing context info with the logging scopes. Tracing context info is the trace Id, Span Id, Parent Id, Trace State, and Trace flags which included inside the Activity object stored in the current execution context. This issue is to support adding the tracing context info with the logging scopes.
Adding Tracing context info to the logging scope will require some memory allocations and also will increase the produced text inside the logs, we are going to provide an opt-in option to enable the new behavior. Although the whole feature is about internal implementation details, we still need to expose some API for the opt-in option. All Proposed APIs here are only to provide the opt-in option for enabling the trace context info in logging and choose which parts of the trace context should be included.
Here is a code sample showing how to opt-in to include the trace context info in the logs.
When enable trace context info logging, will get in the logging results like the following:
Proposed API
The text was updated successfully, but these errors were encountered: