RFC for providing logger information from runsettings #111

abhishkk · 2018-01-09T12:10:34Z

RFC is about adding support for providing logger information from the runsettings.

This reverts commit 1969b24.

codito · 2018-01-09T14:13:06Z

RFCs/0016-Loggers.md

+
+    ```xml
+    <RunSettings>
+        <LoggerRunSettings>


What is the purpose of LoggerRunSettings node? Did we consider RunSettings > Loggers > Logger?

LoggerRunSettings node is added to be consistent with the way data collectors are specified in run settings.

Makes sense. Thanks for keeping these consistent.

codito · 2018-01-09T14:14:35Z

RFCs/0016-Loggers.md

+
+## Summary
+This note outlines the proposed changes for:
+1. Enabling logger support in test platform for TranslationLayer clients.


We support both protocol based and C# library based clients, right?

This will allow developers to specify trx or xunit or nunit loggers from Test Explorer?

Correct. Changes will enable logger support for both protocol based and C# library based clients. I will correct the RFC.

No, user can't specify loggers from test explorer. Loggers can be specified from command line and run settings.

Doesn't VS allow specifying runsettings through Test | Test Settings menu? Do we block the flow anywhere if user specifies loggers in the runsettings there?

Loggers are supported in run settings from Test settings menu also.
Earlier, I thought the question was about some first class support for adding loggers directly from test explorer (and not run settings)

codito · 2018-01-09T14:16:11Z

RFCs/0016-Loggers.md

+
+2. Allowing users to provide loggers from the runsettings enables:
+  * TranslationLayer clients to provide loggers to the test platform.
+  * Alternative way to provide loggers in addition to the existing command line logger argument.


Config as code. Users can commit a runsettings with required loggers and it doesn't need to be specified at each invocation.

thanks. Will correct RFC

codito · 2018-01-09T14:18:00Z

RFCs/0016-Loggers.md

+## Specifying a logger
+Loggers can be provided to the test platform in one of the following ways:
+
+1.  "/Logger:"UriOrFriendlyName";Key1=Value1;Key2=Value2" This is a switch to vstest.console.exe that feeds logger information to the test platform. For instance:


Does this change anything w.r.t loggers specified in dotnet test?

No. Command line behavior (vstest.console and dotnet test) is not changed.

codito · 2018-01-09T14:19:07Z

RFCs/0016-Loggers.md

+1. If same logger is specified both in command line and run settings, command line takes precedence.
+2. Multiple Loggers can be added in runsettings by adding additional Logger node in LoggerRunSettings.Loggers section.
+3. Configuration is optional in Logger node.
+4. Atleast one attribute among uri, friendlyName, assemblyQualifiedName should be present in Logger node.


nit: suggest using inline quotes for readability. E.g. Atleast one attribute among uri, friendlyName, assemblyQualifiedName should be present in Logger node.

Thanks. will make corrections.

codito · 2018-01-09T14:19:45Z

RFCs/0016-Loggers.md

+    <RunSettings>
+        <LoggerRunSettings>
+            <Loggers>
+                <Logger friendlyName="sampleLoggerwithParameters">


What are the errors user will see if a logger is incorrectly specified? Will the test run continue or it will be aborted?

Exceptions are thrown in following scenarios:

LoggerRunSettings or Loggers node has attributes.

LoggerRunSettings has any node other than Loggers.

Loggers has any node other than Logger.

Logger has any node other than Configuration.

Logger node has any attribute other than uri, assemblyQualifiedName, friendlyName, or enabled.

Invalid format uri is given.

Unable to find logger using precedent attribute.

If none of the attributes uri, assemblyName, friendlyName is given.

Warning are printed in following scenarios:

Key value pair has empty or whitespace value. Example: <Key1></Key1>.

Duplicate keys in configuration is ignored and verbose is added for it.

I will add these details in RFC as well.
In addition, Logger node understands enabled attribute as well. I will update RFC regarding the same.

Assuming exceptions will abort the test run.

Yes, exceptions will abort the test run.

What I recall is that IDE also tries to read the runsettings. If something is wrong with settings, IDE aborts the run and request does not reach the TestPlatform.
Also make sure we catch these exceptions, give out an actionable message to user and dont crash vstest.console exe.

In reply to: 160580427 [](ancestors = 160580427)

Sure. I will ensure, that vstest.console does not crash and only test aborts.

codito · 2018-01-09T14:23:01Z

RFCs/0016-Loggers.md

+
+2. TestLoggerManager will implement both ITestRunEventsHandler and ITestDiscoveryEventsHandler2. On receiving any run/discovery event, corresponding logger event will be triggered by TestLoggerManager.
+
+3. ProxyOperationManager will manage TestLoggerManager as loggers can register to both execution and discovery events. ProxyOperationManager will do following to manage TestLoggerManager:


What are the alternatives considered?

Isn't ProxyOperationManager only responsible for initialization of the host and establishing the communication channel? Why is the proxy operation manager the right layer to add this logic?

Isn't managing extensions responsibility of Test Engine?

Test run events are not propagated to test engine. ProxyExecutionManager receives these events. For data collectors also, we have thus created ProxyExecutionManagerWithDataCollection.

In proposed solution, ProxyExecutionManager will call TestLoggerManager events in addition to TestRunRequest events whenever event is received.

As TestLoggerManager needs to listen to both run events and discovery events, I am proposing it to be managed by ProxyOperationManager (as its the parent class).

Rather than considering ProxyOperationManager as only responsible for initialization of the host and communication channel, is it ok to consider this class as parent of ProxyExecutionManager and ProxyDiscoveryManager and thus add common functionality in it?

Thank you for clarifications.

For data collectors also, we have thus created ProxyExecutionManagerWithDataCollection.

This is a decorator, in my opinion. It enriches existing functionality maintaining layering intact. E.g. the inner concerns ProxyExecutionManager or ProxyDiscoveryManager or ProxyOperationManager don't know if there's something called DataCollection. DC is a higher level concept.

As TestLoggerManager needs to listen to both run events and discovery events, I am proposing it to be managed by ProxyOperationManager (as its the parent class).
Rather than considering ProxyOperationManager as only responsible for initialization of the host and communication channel, is it ok to consider this class as parent of ProxyExecutionManager and ProxyDiscoveryManager and thus add common functionality in it?

It is not a good idea. We have to be careful with inheritance, adding functionality like so will break SRP. Also coupling of lower layers with external concerns will increase failure conditions. Please consider making it aspect based, or expose generic capabilities in the lower layers, and compose them. E.g. the lower layer can let the higher ones know when there are interesting events. Higher layer can do any modifications required, further forward them to consumers (extensions). Lower layer is not aware of loggers or any extensions, higher layer does. In case no loggers are specified, higher layer is no-op.

As I understand, there are already events on raw message (or deserialized message) received. Could we modify this, or may be listen to these and say raise events for the extensions to consume?

The pipelines for Discovery and Execution are separate today and merge only at the RequestSender. Suggest to keep it separate, so that they can evolve independently. Discovery/Execution related logger events could be part of their individual flows.

Thanks for pointing the SRP break and suggestions. Can you help in clarifying few points?

Clarifications needed:

Even tough ProxyExecutionManagerWithDataCollection maintains layering, I think it doesn't maintain is a relationship with ProxyExecutionManager. Data collectors doesn't extend/enrich the responsibility of ProxyExecutionManager and have totally different responsibility. The idea of adding logger extension in ProxyOperationManager was to maintain has a relationship. Please correct me if I am not understanding this properly.

I like the approach of raising events from low layer to higher layer for logger extensions. Should we plan in future for moving data collectors also in similar fashion (Considering 1st point correct)?

Proposal as per suggestions:

Does following seems ok?

Low layer (ProxyExecutionManager, ProxyDiscoveryManager) will hold list of event handlers and will raise events to all of these handlers whenever an event is received (Currently TestRunRequest's event handler is hold by this low layer).

This list of event handlers will be passed to ProxyExecutionManager while calling its StartTestRun method. So API of IProxyExecutionManager will change from int StartTestRun(TestRunCriteria testRunCriteria, ITestRunEventsHandler eventHandler); to int StartTestRun(TestRunCriteria testRunCriteria, List<ITestRunEventsHandler> eventHandlers);

TestLoggerManager instance will be created in TestRunRequest constructor.

Whenever TestRunRequest.ExecuteAsync is invoked, TestRunRequest will pass TestLoggerManager event handler (along with its own handler) to ProxyExecutionManager.StartTestRun

Whenever TestRunRequest is disposed, TestLoggerManager's logger events will also be disposed.

This way low layer will have idea only about event handlers (and not extensions, loggers etc)

Even though ProxyExecutionManagerWithDataCollection...

It actually ends up implementing the same IProxyExecutionManager interface via ProxyExecutionManager inheritance. There's is a relationship. The enrichment happens in two ways: before starting the test run ensure datacollectors are started, and after end of the test run ensure datacollector attachments are collected. Apart from these two, a PEMWithDataCollection uses PEM for actual execution.

...Should we plan in future for moving data collectors also in similar fashion

It should be possible depending on granularity of events.

Proposal...

Let's chat a bit to discuss the pros and cons.

Another approach: there could be a new internal TestRunEvents entity which exposes various run related events. Interested parties like extensions/loggers etc. subscribe to it. We can provide an updated ITestRunEventsHandler implementation which broadcasts the events in TestRunEvents. Effectively we are getting rid of sending this as the event handlers everywhere within the engine.

ITestRunEventsHandler still remains an external concept used widely across translation layer, testhost, engine etc..

Update: As per discussion with Arun, we are managing TestLoggerManager from TestRunRequest now. TestPlatform will create the TestLoggerManager and initialize it.
TestRunRequest will hold the instance of TestLoggerManager and call its method whenever an event is recevied. On test run completionm testRunRequest will dispose the TestLoggerManager.

codito · 2018-01-09T14:23:30Z

RFCs/0016-Loggers.md

+    </RunSettings>
+    ```
+
+    This loads and initializes:


Can you please specify the exact logger APIs and the parameters it will be invoked with (for the above runsettings sample)?

Loggers from run settings mimics the behavior of loggers from command line (from logger initialization perspective).

Still, I will add logger API which will invoked while initialization in RFC. Thanks.

codito · 2018-01-09T14:25:11Z

RFCs/0016-Loggers.md

+                        <Key2>Value2</Key2>
+                    </Configuration>
+                </Logger>
+                <Logger uri="logger://sample/sampleLoggerWithoutParameters1"


Can user configure console logger in same way?

Yes. user can configure any logger including console in this way.

codito · 2018-01-09T14:26:54Z

RFCs/0016-Loggers.md

@@ -0,0 +1,85 @@
+# 0016 Loggers


Request to make the title and doc name little more elaborate. This may help understand the intent from looking at filename.

Sure. Will elaborate the title.

codito · 2018-01-09T14:28:30Z

RFCs/0016-Loggers.md

+    This loads and initializes:
+    * Logger with friendlyName="sampleLoggerwithParameters". Key1=Value1 and Key2=Value2 are passed as dictionary parameters to the logger while initialization.
+    * Logger with uri="logger://sample/sampleLoggerWithoutParameters1". FriendlyName is ignored in this case as uri takes more precedence.
+    * Logger with assemblyQualifiedName="Sample.Sample.Sample.SampleLogger, Sample.Sample.Logger, Version=0.0.0.0, Culture=neutral, PublicKeyToken=xxxxxxxxxxxxxxxx". Uri and friendlyName are ignored in this case as assemblyQualifiedName takes more precedence.


And user can override any runsetting with command line params like so vstest.console foo.dll -- RunSettings.Loggers.Logger. Is this supported? And will the precedence order be respected in this case?

No, this is not supported.

What's the rationale for not supporting it? At the least we shouldn't block this please. It will complicate implementation as I understand.

Sorry for the confusion. User can override logger runsetting from command line paramas. I will update this info in RFC. thanks

codito · 2018-01-09T14:29:34Z

RFCs/0016-Loggers.md

+                    <Configuration>
+                        <Key1>Value1</Key1>
+                        <Key2>Value2</Key2>
+                    </Configuration>


Did we consider passing the entire runsettings to each logger? This will allow a logger to have any custom parameter as required (similar to Datacollectors).

This requires API changes in logger extensibility. We are not taking this change as part of this RFC. But we are taking key values in configuration element from runsettings thinking these changes in mind.

Makes sense.

codito · 2018-01-09T14:29:53Z

RFCs/0016-Loggers.md

+
+## Design
+Following are the proposed design changes for enabling logger support for TranslationLayer clients:
+1. TestLoggerManager will manage all the loggers.  TestLoggerManager will be responsible for:


Does it include console logger as well?

TestRequestManager will add console logger node in runsettings with assemblyQualifiedName attribute pointing to console logger type.
TestLoggerManager reads runsettings and initializes logger based on assemblyQualifiedName, uri and friendlyName.

Makes sense.

codito · 2018-01-11T13:47:50Z

RFCs/0016-Loggers-Information-From-RunSettings.md

+2. TestPlatform will create and initialize TestLoggerManager and will pass this TestLoggerManager instance to TestRunRequest and DiscoveryRequest.
+3. TestRunRequest and DiscoveryRequest will hold the TestLoggerManager instance. On receving any run/discovery events, respective methods of TestLoggerManager will be invoked. i.e. TestLoggerManager will no longer register logger events with run or discovery events.
+4. On test run/discovery completion, TestLoggerManager will be disposed by TestRunRequest and DiscoveryRequest.
+5. Limitaion with this approach is that if TestRunRequestusers like TestRequestManager tries to call `TestRunRequest.ExecuteAsync()` second time, then logger events will not be invoked as TestLoggerManager will already be disposed in first run.


s/Limitaion/Limitation

s/TestRunRequestusers

In reply to: 160961891 [](ancestors = 160961891)

This is interesting. What is the plan if we need this in future ?

In reply to: 160987392 [](ancestors = 160987392,160961891)

For now, we are supporting loggers for first test run of a test request. I will add a comment in code as well. It can come as a separate feature work if we add second run scenario.

singhsarab · 2018-01-11T15:36:58Z

RFCs/0016-Loggers-Information-From-RunSettings.md

+  * Disposing logger events.
+
+2. TestPlatform will create and initialize TestLoggerManager and will pass this TestLoggerManager instance to TestRunRequest and DiscoveryRequest.
+3. TestRunRequest and DiscoveryRequest will hold the TestLoggerManager instance. On receving any run/discovery events, respective methods of TestLoggerManager will be invoked. i.e. TestLoggerManager will no longer register logger events with run or discovery events.


receving [](start = 84, length = 8)

s/receving/receiving

singhsarab

Signing off with suggestions.

abhishkk added 5 commits January 9, 2018 17:30

Logger RFC

1969b24

Revert "Logger RFC"

052f3c8

This reverts commit 1969b24.

Logger RFC

1b58d6b

spacing issue

80e6ddd

spacing

73d1824

codito reviewed Jan 9, 2018

View reviewed changes

review comments

7e7bfe8

codito reviewed Jan 11, 2018

View reviewed changes

codito approved these changes Jan 11, 2018

View reviewed changes

singhsarab reviewed Jan 11, 2018

View reviewed changes

singhsarab approved these changes Jan 11, 2018

View reviewed changes

abhishkk added 3 commits January 12, 2018 09:15

Spell corrections

7e52d83

correction in design

9904f8b

spell correction

413b94f

singhsarab merged commit b805f3c into microsoft:master Jan 12, 2018

abhishekkumawat23 mentioned this pull request Jan 22, 2018

Logger support in run settings microsoft/vstest#1382

Merged


		2. TestLoggerManager will implement both ITestRunEventsHandler and ITestDiscoveryEventsHandler2. On receiving any run/discovery event, corresponding logger event will be triggered by TestLoggerManager.

		3. ProxyOperationManager will manage TestLoggerManager as loggers can register to both execution and discovery events. ProxyOperationManager will do following to manage TestLoggerManager:

RFC for providing logger information from runsettings #111

RFC for providing logger information from runsettings #111

Conversation

abhishkk commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Clarifications needed:

Proposal as per suggestions:

codito Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab left a comment

Choose a reason for hiding this comment

abhishkk Jan 10, 2018 •

edited

Loading

codito Jan 10, 2018 •

edited

Loading

abhishkk Jan 9, 2018 •

edited

Loading

abhishkk Jan 9, 2018 •

edited

Loading