serilog/serilog-sinks-opentelemetry

Include unit tests for .NET framework and instructions for setting up activity traces

alsi-lawr opened this issue · 4 comments

alsi-lawr commented

Description
I've run the unit tests against all supported .NET versions (netstandard2.0, net471, net462, net6, net8) and noticed a discrepancy. Activity traces and spans aren't populated in net471 and net462.

Reproduction
Run the unit tests against all supported .NET versions.

If it's a core issue with the .NET framework runtimes, I'd suggest just making a note that traceids and spanids aren't supported in .NET framework. Otherwise, it's something to look into.

nblumhardt commented

Thanks for the note! Tracing definitely does work on these platforms, the problem will be that if you port the tests to run on .NET Framework, you'll need to set the default activity id format before any Activity instances are constructed.

Here's how SerilogTracing's configuration process does it:

https://github.com/serilog-tracing/serilog-tracing/blob/dev/src/SerilogTracing/ActivityListenerConfiguration.cs#L62

Not sure whether there's much to win in the Serilog.Sinks.OpenTelemetry tests by adding a .NET Framework, but the CI process does currently support .NET Framework so we could definitely consider it.

alsi-lawr commented

Thanks for the note! Tracing definitely does work on these platforms, the problem will be that if you port the tests to run on .NET Framework, you'll need to set the default activity id format before any Activity instances are constructed.

Here's how SerilogTracing's configuration process does it:

https://github.com/serilog-tracing/serilog-tracing/blob/dev/src/SerilogTracing/ActivityListenerConfiguration.cs#L62

Not sure whether there's much to win in the Serilog.Sinks.OpenTelemetry tests by adding a .NET Framework, but the CI process does currently support .NET Framework so we could definitely consider it.

As long as it's tested in some form and future developers are aware that it's not covered by unit tests, I don't see a reason to add it to the unit tests. Perhaps just a note in the README about how to set it up in .NET framework would be nice. I have no horse in the race since I'm not even using .NET framework.

Although it wouldn't hurt to just have in the tests:

#if NETFRAMEWORK
//... setup activity default
#endif

I wouldn't mind adding it to the unit tests and submitting a PR if you want to go that route, your call.

nblumhardt commented

If you're keen, a PR adding net4.x targets to the tests would be welcome. I'm assuming this won't require much noise in the tests so please do send a heads-up if it looks like any major changes are required :-)

alsi-lawr commented

Hey @nblumhardt, I've just submitted the PR for those changes with minimal issues

  • 👍1
  • 🎉1