Draft Proposal for Diagnostics Client Library (a.k.a. "Runtime Client Library") #574
Conversation
|
The lack of var in the sample code makes this feel like java. |
| using System.IO; | ||
| using System.Threading.Task; | ||
|
|
||
| public void TraceProcessForDuration(int processId, int duration, string traceName) |
| source.Process(); | ||
| } | ||
| // NOTE: This exception does not currently exist. It is something that needs to be added to TraceEvent. | ||
| catch (EventStreamException e) |
There was a problem hiding this comment.
Do we need to create a new exception or could we just catch the exception that caused the stream reading error? I don't want to hide the underlying issue by having the EventSource simply say "something went wrong".
There was a problem hiding this comment.
By EventSource do you mean EventPipeEventSource?
There was a problem hiding this comment.
could we just catch the exception that caused the stream reading error
This part is a little ambiguous. I think of this EventStreamException as an exception that gets thrown by EventPipeEventSource when there is an exception while reading the stream because either the stream was shut down by something. There could be other exception, like incorrect or corrupted payload, which can probably be encapsulated by a different exception class.
I don't want to hide the underlying issue
The sample is meant to capture the intention to add more exception classes to EventPipeEventSource to make it not throw general Exception to prevent exactly this kind of things from happening.
| ///<summary> | ||
| /// Stops the given session | ||
| ///</summary> | ||
| public void Stop(); |
There was a problem hiding this comment.
Without a constructor or initialize method, how will Stop() know what session id to send?
There was a problem hiding this comment.
The constructor is internal. _sessionId is a private member - the goal here is to provide as simple of an API as possible for the end user. Internal states like sessionId is either internal/private and won't be exposed unless we see a need for it.
This is a draft proposal for the diagnostics client library - which we've been calling the "runtime client library" till now - (yes the rename is part of the proposal but I really want us to stop calling it the runtime client library :p).
Some of the sample code is still left TODO, but I think this is at a point where we can openly start discussing about the design choices.
cc @noahfalk @josalem @mikem8361 @tommcdon