Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft Proposal for Diagnostics Client Library (a.k.a. "Runtime Client Library") #574
Draft Proposal for Diagnostics Client Library (a.k.a. "Runtime Client Library") #574
Changes from all commits
c8d1da2
5aa58a5
b14c3dd
724ca08
e03a50a
3ac65ca
09edb81
6db8055
8ffcb28
1ab8edb
4fe23b3
8c249ab
8938d93
00ca2be
98f5509
100cec0
8c8281b
cf5b24d
1859bf8
b121b5f
9f671e8
db05fb7
8148b9f
32b5a64
738a947
2d6a498
9d7bac2
7eb973a
9bc8658
57d4f0b
aa4169d
74d9e12
1038447
c05e35c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By
EventSource
do you meanEventPipeEventSource
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops! Yeah, I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make duration a TimeSpan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without a constructor or initialize method, how will
Stop()
know what session id to send?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.