"Invalid Character" when using Scope names with state

[WolfspiritM]

Please mark the type framework used:

• ASP.NET MVC
• ASP.NET Web API (OWIN)
• ASP.NET Core
• WPF
• WinForms
• Xamarin
• Other:

Please mark the type of the runtime used:

• .NET Framework
• Mono
• .NET Core

• Version:

Please mark the NuGet packages used:

• Sentry
• Sentry.Serilog
• Sentry.NLog
• Sentry.Log4Net
• Sentry.Extensions.Logging
• Sentry.AspNetCore

• Version: 2.1.8

When using the AspNetCore ILogger like this:

using (logger.BeginScope("HTTP {HttpMethod} {Uri}", "123", "1234")){
	logger.LogCritical("Test");
}

This creates a scope called "HTTP 123 1234" and additional tags "HttpMethod: 123" and "Uri: 1234".
Sentry successfully gets the tags but the scope results in an error in the sentry frontend like this:

The tags json will contain:

{"value":"HTTP {HttpMethod} {Uri}","key":null}

I think Sentry should be able to use "HTTP 123 1234" as value and probably have "scope" as a tag key (Are arrays supported as tag keys?)

[bruno-garcia]

ILogger doesn't have an overload with two arguments as in your example.

So you must have your custom extension method that does something with those two arguments and pass it to the BeginScope<TState>. What does that code look like?

You can add your custom code to apply TState to the Sentry Scope.

Through: ScopeOptions.SentryScopeStateProcessor.

An example here:

sentry-dotnet/samples/Sentry.Samples.Console.Customized/Program.cs

Lines 272 to 287 in e17703a

	private class MyCustomerScopeStateProcessor : ISentryScopeStateProcessor
	{
	private readonly ISentryScopeStateProcessor _fallback = new DefaultSentryScopeStateProcessor();
	public void Apply(IScope scope, object state)
	{
	if (state is SpecialContextObject specialState)
	{
	scope.SetTag("SpecialContextObject", specialState.A + specialState.B);
	}
	else
	{
	_fallback.Apply(scope, state);
	}
	}
	}

[WolfspiritM]

@bruno-garcia Ilogger itself does not but the LoggerExtensions do:

https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerExtensions.cs#L410

[bruno-garcia]

Oh that's new I believe? I wasn't aware of that extension method anyway.

It basically: new FormattedLogValues(messageFormat, args) so uses the default formatter thing.

Sounds like we'd need to special case for that, and extract the key from the message format (like {a} would be key a) to add the tags.

The code would go here:

sentry-dotnet/src/Sentry/Protocol/DefaultSentryScopeStateProcessor.cs

Line 17 in e17703a

switch (state)

We could test that with a unit test, calling the same extension method you did. In fact, your use case could be the test case.

Is this something you'd be willing to contribute with a PR?

[WolfspiritM]

Sounds like we'd need to special case for that, and extract the key from the message format (like {a} would be key a) to add the tags.

That actually already works. With the example above I get "HttpMethod" as Key and "123" as value but the message itself is passed without any kind of replacement and any kind of key which marks it as invalid in the frontend. I guess the tags work cause FormattedLogValues is based on a IReadOnlyList<KeyValuePair<string, object>>

I can take a look if I can get a PR ready 😄

[WolfspiritM]

@bruno-garcia I've been digging a bit more. The reason this fails is that FormattedLogValues adds the original message as another item to the keyvaluepairlist with key "{OriginalFormat}".

So the problem is not the "{" in the message but the "{" in the Key that is added.

My test case is : "scope {variable}", "test"

I can see multiple ways to handle this but I'm not sure what might be the best:

1. Replace { and } in all keys so sentry would display "OriginalFormat: scope {variable}"
2. Remove any entry that is called "{OriginalFormat}" completly.
3. If {OriginalFormat} exists then I can add a "Scope" key with value of keyValStringObject.toString() which in this case is the formatted output: "scope test"

In my opinion "OriginalFormat: scope {variable}" doesn't hold any information so maybe a combination of 2 and 3 is fine?
In my test case it would result in two tags:
Scope: scope test
variable: test

Would that be acceptable? I'm not sure about the name "Scope" maybe "ActiveScope", "ScopeMessage" or something completly different would be better?

EDIT: Oh and I sadly can't check if it's FormattedLogValues as that type is internal since .NET Core 3 it seems 😞
See also this comment in the test:

sentry-dotnet/test/Sentry.Extensions.Logging.Tests/MelDiagnosticLoggerTests.cs

Lines 45 to 48 in 234f34c

	// .NET Core 3 (and hence .NET 5) has turned FormattedLogValues into an internal readonly struct
	// and now we can't match that with NSubstitute
	#if NETCOREAPP2_1 || NET461
	[Fact]

[bruno-garcia]

We have a special case for it too here:

sentry-dotnet/src/Sentry.Extensions.Logging/SentryLogger.cs

Line 70 in d44caeb

if (property.Key == "{OriginalFormat}" && property.Value is string template)

In this case ideally we'd add the formatted version only. I've used scope as key so might conflict with other things.

[fearhq]

I am experiencing this exact problem too. We have recently updated our asp .net core version to 3.1 and sentry to 2.1.8