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

Trace fields are seems to be incorrect #44

Closed
StasPerekrestov opened this issue Mar 15, 2021 · 7 comments
Closed

Trace fields are seems to be incorrect #44

StasPerekrestov opened this issue Mar 15, 2021 · 7 comments
Assignees

Comments

@StasPerekrestov
Copy link
Contributor

StasPerekrestov commented Mar 15, 2021

Hello,
I've found out that according to

https://cloud.google.com/trace/docs/trace-log-integration

traces should have the following format
projects/[PROJECT_ID]/traces/[TRACE_ID]
however, in LogFormatter.cs code

private void CheckForSpecialProperties(LogEntry log, string key, string value)
{
    if (_useSourceContextAsLogName && key.Equals("SourceContext", StringComparison.OrdinalIgnoreCase))
        log.LogName = CreateLogName(_projectId, value);

    if (_useLogCorrelation)
    {
        if (key.Equals("TraceId", StringComparison.OrdinalIgnoreCase))
            log.Trace = $"projects/{_projectId}/traces/{key}";

        if (key.Equals("SpanId", StringComparison.OrdinalIgnoreCase))
            log.SpanId = key;
    }
}

produces Trace references like projects/vitupay-oos/traces/TraceId
SpanIds are also suspicious.

As I understand, it should be

  log.Trace = $"projects/{_projectId}/traces/{value}";
...
  log.SpanId = value;
@StasPerekrestov
Copy link
Contributor Author

Basically, it results in
image

@manigandham
Copy link
Owner

thanks for reporting @StasPerekrestov

fixed now in version 3.3.3: https://www.nuget.org/packages/Serilog.Sinks.GoogleCloudLogging/3.3.3

@StasPerekrestov
Copy link
Contributor Author

Thank you so much!

@arakis
Copy link
Contributor

arakis commented Mar 18, 2021

In my initial PR, i used that piece of code (works / tested):

// Maybe we could cache this, but the traceId will change often
entry.Trace = $"projects/{_projectId}/traces/{traceId.StringValue}";
propStruct.Fields.Remove("TraceId");
...
entry.SpanId = spanId.StringValue;
propStruct.Fields.Remove("SpanId");

It seems, it went broken after merge & refactor:
613938d#diff-b9ef05e354948587d0455408b01f2475f65ce64fee7c14b70d27f855b330ba2cR170

But glad to see, that @manigandham fixed that so fast :-)

Thanks!! :-)

@StasPerekrestov
Copy link
Contributor Author

I'm not sure if it should be part of this repository,
there is an option to get information logs in GCP Trace List using IManagedTracer.

It's something that doesn't work for me because of:
serilog/serilog-aspnetcore#207

Also, please note there are some behavioral differences between asp.net core 3.1 and 5.0, which should be taken into consideration.

internal class GoogleTraceActivityEnricher : ILogEventEnricher
	{
		private readonly IHttpContextAccessor _httpContextAccessor;

		public GoogleTraceActivityEnricher(IHttpContextAccessor httpContextAccessor)
		{
			_httpContextAccessor = httpContextAccessor;
		}

		public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory)
		{
			HttpContext? ctx = _httpContextAccessor.HttpContext;
			if (ctx == null)
			{
				Activity? activity = Activity.Current;

				if (activity != null)
				{
					//..fallback: grab values from Activity: TraceId, SpanId, ParentId
				}
			}
			else
			{
				var managedTracer = ctx.RequestServices.GetRequiredService<IManagedTracer>();
				string? traceId = managedTracer.GetCurrentTraceId();
				ulong? spanId = managedTracer.GetCurrentSpanId();
				//in .net 3.1 TraceId/SpanId are propagated automatically and we need to override it with Google TraceId 
				if (traceId != null)
				{
					logEvent.AddOrUpdateProperty(new LogEventProperty("TraceId", new ScalarValue(traceId)));
				}
				if (spanId != null)
				{
					logEvent.AddOrUpdateProperty(new LogEventProperty("SpanId", new ScalarValue(spanId)));
				}
			}
		}
	}

@manigandham FYI :)

@manigandham
Copy link
Owner

Hey @StasPerekrestov

This repo is just for the sink which sends the logs to GCP. Adding the activity ids is the responsibility of the upstream logging provider. It's only an issue because serilog doesn't (yet) use LoggerExternalScopeProvider but you can already add the enricher to handle that: https://github.com/RehanSaeed/Serilog.Enrichers.Span

@StasPerekrestov
Copy link
Contributor Author

Yeah, sorry, I perhaps confused you. I referenced the issue just to illustrate why some workarounds exist in the code above.

If I use UseGoogleTrace extension method from Google SDK to add tracing middleware, it doesn't correlate logs with Google Traces. As a result, I used the above code (it uses IManagedTracer) to propagate Google's TraceId and SpanId into Logs.

it allows seeing information logs in Trace List:

image

@manigandham manigandham self-assigned this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants