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

Complex objects property expansion #57

Open
unsafePtr opened this issue Feb 3, 2025 · 6 comments
Open

Complex objects property expansion #57

unsafePtr opened this issue Feb 3, 2025 · 6 comments

Comments

@unsafePtr
Copy link
Contributor

Background

When a complex object is submitted as an array/class, it's not logged properly, as the logger message by default calls ToString() on the object.

Repdocution

You can reproduce the issue from this branch
https://github.com/purview-dev/purview-telemetry-sourcegenerator/compare/main...unsafePtr:purview-telemetry-sourcegenerator:complex-object-reproduce?expand=1

Solution

One way is to use always records instead of class for the logging since ToString is overrided for them. However, if an array/list/dictionary is submitted, it won't take any effect. Since .net8, there has been a LogProperties attribute, but it seems it doesn't take any effect when added.

@kieronlanning
Copy link
Contributor

Looking at this, I wonder if my generator should attempt to generate the partial methods instead (including passing on the LogPropertiesAttribute), allowing the existing generators to take over and do its thing.

I'm not sure if that's possible or not, but I'll have a play around and see. If not, then I'll look into support.

@unsafePtr
Copy link
Contributor Author

Seems it's not supported, as you can set in which order source generators should be executed.
dotnet/roslyn#57239

That's not a must. Feel free to close issue, as in theory developer can always define custom class with array field and overried ToString.

@kieronlanning
Copy link
Contributor

I checked the same after you posted the issue - it's something I'd like to investigate anyway; I can see the value.

I'll have a think about it.

@kieronlanning
Copy link
Contributor

BTW, I've not run your sample yet, but I'm assuming the complex object is just ToString'd, so the title of the issue is actually about property expansion?

@unsafePtr
Copy link
Contributor Author

@kieronlanning yes, it's basically about property expansion. As I've said even now you can work with it by overriding ToString. The problem comes when you need to use object from 3rd party library. However, you can always create custom object and map it 1:1 to existing and override ToString. That's what I did actually 😃

@unsafePtr unsafePtr changed the title Complex objects are not logged Complex objects property expansion Feb 3, 2025
@kieronlanning
Copy link
Contributor

kieronlanning commented Feb 4, 2025

My initial thoughts are:

If the LogPropertiesAttribute type is available

Which is via the Microsoft.Extensions.Telemetry.Abstractions assembly, generate code that's similar to the output from the Microsoft.Gen.Logging source generator.

An example of this is:

// Source

[LoggerMessage(
	Level = LogLevel.Debug,
	Message = "Generating {Forecast}")]
private static partial void Complex_Default(ILogger logger, [LogProperties] WeatherForecast forecast);

// Generates

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Gen.Logging", "9.1.0.0")]
private static partial void Complex_Default(global::Microsoft.Extensions.Logging.ILogger logger, global::WeatherForecast forecast)
{
    if (!logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Debug))
    {
        return;
    }

    var state = global::Microsoft.Extensions.Logging.LoggerMessageHelper.ThreadLocalState;

    _ = state.ReserveTagSpace(6);
    state.TagArray[5] = new("Forecast", forecast);
    state.TagArray[4] = new("forecast.Date", forecast?.Date);
    state.TagArray[3] = new("forecast.TemperatureC", forecast?.TemperatureC);
    state.TagArray[2] = new("forecast.Summary", forecast?.Summary);
    state.TagArray[1] = new("forecast.TemperatureF", forecast?.TemperatureF);
    state.TagArray[0] = new("{OriginalFormat}", "Generating {Forecast}");

    logger.Log(
        global::Microsoft.Extensions.Logging.LogLevel.Debug,
        new(1730470092, nameof(Complex_Default)),
        state,
        null,
        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Gen.Logging", "9.1.0.0")] static string (s, _) =>
        {
            var forecast = s.TagArray[5].Value ?? "(null)";
            #if NET
            return string.Create(global::System.Globalization.CultureInfo.InvariantCulture, $"Generating {forecast}");
            #else
            return global::System.FormattableString.Invariant($"Generating {forecast}");
            #endif
        });

    state.Clear();
}

The types required for this type of generation are only available in the Microsoft.Extensions.Telemetry.Abstractions, so I can't rely on the present of the ILogger type which is what I've been using.

However, if the consumer pops the LogPropertiesAttribute on a parameter then we're good to go.

If it's not available

...fallback to the way I'm currently generating the logging output, which is using the LoggerMessage.DefineX methods.

This is limited to 6 parameters anyway, so there's little point in adding anything special to support LogPropertiesAttribute.

What I'd like to do...

... is support both.

And then provide additional support for outputting arrays/ enumerables effectively but with a massive diagnostics warning that the consumer will need to disable. You can read more about this particular issue/ design choice here.

Of course it would include support for the OmitReferenceName and SkipNullProperties properties.

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

2 participants