Orleans: Implicit dependency of Testing host on OrleansTelemetryConsumers.Counters

[centur]

Hi, I'm doing nuget upgrade 1.2.3->1.3.0 for all Orleans package, and in unit tests I got this exception:

System.Exception
Exception during test initialization: 
Exc level 0: System.IO.FileNotFoundException: Could not load file or assembly 'OrleansTelemetryConsumers.Counters, Version=1.3.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The system cannot find the file specified.
   at Orleans.TestingHost.TestingSiloHost.RegisterPerfCountersTelemetryConsumer()
   at System.AppDomain.DoCallBack(CrossAppDomainDelegate callBackDelegate)
   at Orleans.TestingHost.TestingSiloHost.LoadSiloInNewAppDomain(String siloName, SiloType type, ClusterConfiguration config, AppDomain& appDomain)
   at Orleans.TestingHost.TestingSiloHost.StartOrleansSilo(TestingSiloHost host, SiloType type, TestingSiloOptions options, Int32 instanceCount, AppDomain shared)
   at Orleans.TestingHost.TestingSiloHost.<InitializeAsync>d__72.MoveNext()
   at Orleans.TestingHost.TestingSiloHost.DeployTestingSiloHost(TestingSiloOptions siloOptions, TestingClientOptions clientOptions)
   at DBCloud.ActorTests.UnitTestSiloHost..ctor() in 

Checked the nuget and I haven't found any explicit nuget dependencies on this package ( also I believe it didn't exist before 1.3.0
Adding this package to test project makes it working again, although it's a bit weird solution. I checked the dependencies on nuget.org and can't find any explicit package dependency of testing host nuget on telemetrycounters. Is this a bug ?

PS: just discovered that there was another very recent release 1.3.1, since I updated most of the packages to 1.3.0. Consolidating the packages and moving everything to 1.3.1. Still it looks like a dependency is missing

[centur]

The problem is still there, upgraded to 1.3.1 and checked it with the latest versions - still the same error, simple to fix by adding missing nuget manually to the project.

But the right fix is to declare necessary dependencies on the nuget.

cc @galvesribeiro

[xiazen]

Hi @centur

Thanks for submitting this issues. I think this is a bug. Microsoft.Orleans.OrleansHost nugget should have a dependency on OrleansTelemetryConsumers.Counters package, since TestCluster uses its OrleansPerfCounterTelemetryConsumer as a TelemetryConsumer. I think we should submit a PR to fix this.

@sergeybykov @jdom (happy vacation) what do you think?
if you second my opinion I will go ahead and submit a PR.

[galvesribeiro]

@xiazen go ahead and submit it. I though it was fixed. However, We should eventually remove that dependency from Microsoft.Orleans.OrleansHost since performance counters are totally optional now.

@jason-bragg made all the remaining counters from Orleans to pass thru the Telemetry APIs recently, so if people want counters, add it. Otherwise, any registered consumer(or none!) should receive the same counters so no point on depend on it IMHO.

[xiazen]

@galvesribeiro yes I was just thinking about that.Thanks for replying.

Are we ready to remove dependency on OrleansPerfCounterTelemetryConsumer now? Since in my understanding of telemetry's current state, we can remove that dependency, and if user want telemetry consumer to consume Orleans counter output, they can register their own telemetry consumer, and that consumer will get all counter output, due to Jason's recent work.

[galvesribeiro]

@xiazen yep, that is the general intention of those APIs. Unless somewhere in the code still any direct reference to counters, you can remove it.

[jdom]

Haven't read the entire thread, but as far as the dependency from the testing host to the counters, it should be removed from the code (instead of adding it as a user package dependency). When @galvesribeiro removed the counters from the main assembly, we temporarily added the telemetry consumer to make sure we do didn't break anything and let it run for some time. I think it's time to remove it from the test host entirely.

[galvesribeiro]

Yup, the latest push from Jason removed the remaining deps

[xiazen]

okie dokie. I will submit a PR today . :)

[xiazen]

PR submitted : #2576

[xiazen]

PR merged. close this issue