arcus-azure / arcus.observability

Observability with Microsoft Azure in a breeze.
https://observability.arcus-azure.net/
MIT License
23 stars 15 forks source link

Ambiguous calls while using ILoggerExtensions #263

Closed gverstraete closed 2 years ago

gverstraete commented 2 years ago

So currently I am having a solution with this setup:

Solution.sln SqlProject.csproj using Arcus.observability.Telemetry.Sql (2.3.0) FunctionProject.csproj using SqlProject.csproj using Arcus.Observability.Telemetry.Core (2.3.0)

When trying to use the ILoggerExtensions in the FunctionProject I get this exception: Error CS0433 The type 'ILoggerExtensions' exists in both 'Arcus.Observability.Telemetry.Core, Version=2.3.0.0, Culture=neutral, PublicKeyToken=null' and 'Arcus.Observability.Telemetry.Sql, Version=2.3.0.0, Culture=neutral, PublicKeyToken=null' Hyperion.Platform.DeviceManagement.Monitor C:\Dev\Schreder\exedra-device-management-api\src\Hyperion.Platform.DeviceManagement.Monitor\MessageHandlers\MessageLostV1MessageHandler.cs 33 Active

This totally makes sense as the Sql one is indrectly linked. But how can I fix this? - Just setting the namespace is not enough because it's referring to the same "logical class" as both signatures are the same: https://github.com/arcus-azure/arcus.observability/search?q=ILoggerExtensions

How can I workaround this? - And what was the reasoning for this setup?

Thank you for the feedback!

fgheysels commented 2 years ago

The question is twofold.

1) How can I work around this: I think you should invoke the LogSqlDependency method as a static method instead of an extension method, and specify the complete namespace of the ILoggerExtensions class that you want to use. However, as I'm typing this, it looks like both classes are in the same namespace. Anyway, I'd try that anyway as it looks like the method signatures are not identical ? (Or, you remove the dependency to Telemetry.Sql, and do not call the methods that are defined in there anymore. Looks like they only contain conviency methods which allow you to pass in a connectionstring and that method then extracts the required properties from the connection-string)

2) why is this logic scattered over multiple assemblies / projects. For me, it even looks to be some kind of overkill to have a separate project / assembly / nuget package just for some additional extension methods on ILogger. TBH, I dont see the advantage in this. The more assemblies you need to load, the slower the load-time is. The only reason I can think of, is because these projects (Observability.IoT and Observability.Sql) are introducing dependencies to yet other libraries (DeviceClient & SqlClient), which are only necessary if you're indeed using IoT or SQL related functionality. Therefore, I'd suggest we move the LoqSqlDependency methods that still exist in the 'Core' project to the 'Sql' project. OTOH, then we should need to do this for all depency log methods, if we want to be consistent ....

stijnmoreels commented 2 years ago
  1. I just tried this setup, but didn't came across this issue. Like Frederik said, the signatures are different. Maybe using the right parameter names can also help you specify directly which overload you want. Let us know!
  2. Yes, we have split different dependency tracking in different projects to make sure that we don't include unnecessary packages in the main 'Core' telemetry package. This is done for ASP.NET Core, SQL and IoT dependencies. That doesn't mean that those dependencies can't be tracked only with 'Core', it means that some more tech-specific overloads are located there to help you with your setup (like Frederik realized 😉 ).
gverstraete commented 2 years ago

Let me add some more information.

  1. I do have the issue when using LogMetric since both have these implemented. I don't encounter this at this stage with LogSqlDependency
  2. You are making a very valid point here! When looking at it today it is pretty odd that it uses ILoggerExtensions.LogMetric(...) instead of Logger.LogMetric(...).

I've modified now from this: ILoggerExtensions.LogMetric(Logger, Metrics.DeviceReconnected, 1.0); To this: Logger.LogMetric(Metrics.DeviceReconnected, 1, null);

And now it does resolve for some reason. Will test a bit further with this and update this thread. Both implementations implement the LogMetric in their own library, which is causing this exception to occur. PS: it only works when passing the optional parameters, otherwise I still get the same exception.

stijnmoreels commented 2 years ago

Aha! Great to hear. Yes, let us know.

fgheysels commented 2 years ago

Conclusion: it's not a great developer experience, so this must be investigated and improved.

gverstraete commented 2 years ago

So, it does work like this, but it is indeed not a great experience. If I look at it now, using the logger instead of ILoggerExtensions is the better way anyhow... So I could just close it as well. Open for your feedback.

fgheysels commented 2 years ago

Is this related to #280 ?

gverstraete commented 2 years ago

Yes, it is... This can be closed imo.