Skip to content

[Proof of Concept] Azure Monitor - Refactoring the EventSource#14781

Closed
TimothyMothra wants to merge 7 commits intomasterfrom
tilee/azure_monitor_eventsource
Closed

[Proof of Concept] Azure Monitor - Refactoring the EventSource#14781
TimothyMothra wants to merge 7 commits intomasterfrom
tilee/azure_monitor_eventsource

Conversation

@TimothyMothra
Copy link

@TimothyMothra TimothyMothra commented Sep 1, 2020

Creating this PoC to get feedback on this approach.

The idea behind this change is to have basic Events in the EventSource class to prevent each PR from creating new Event methods.
This should simplify and standardize the way to handle exceptions.

Changes

  • Refactor EventSource class. This introduces one method per severity level, plus an additional method to handle exceptions.

Comment

An earlier version of this was re-throwing the exceptions from the EventSource class, to fully encapsulate the exception handling.
I had this working, but it required extra considerations to preserve the stack trace.
I decided against this approach because it wasn't obvious to myself (and FxCop) that a try/catch block would re-throw any exception.

@reyang
Copy link
Member

reyang commented Sep 2, 2020

@xiang17 need your input when this PR is ready for review

{
AzureMonitorTraceExporterEventSource.Log.ConnectionStringError(ex);
throw new InvalidOperationException("Connection String Error: " + ex.Message, ex);
var newEx = new InvalidOperationException("Connection String Error: " + ex.Message, ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - is this ex.Message localized or not (or not decided at this moment)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know if the ex.Message is ever localized. In theory, exceptions could be thrown from any dependency library so we would have to ask did every library owner localize their messages.
At the moment we're not localizing our own messages.

@jsquire
Copy link
Member

jsquire commented Sep 25, 2020

Hi @TimothyMothra. There hasn't been recent engagement on this PR. Would you please be so kind as to let us know if this is still an active work stream or if the PR should be closed out?

@jsquire
Copy link
Member

jsquire commented Oct 2, 2020

Since there hasn't been recent engagement, I'm going to close this out. Please feel free to reopen if you'd like to continue working on these changes.

@jsquire jsquire closed this Oct 2, 2020
@jsquire jsquire added the no-recent-activity There has been no recent activity on this issue. label Oct 2, 2020
@ArthurMa1978 ArthurMa1978 deleted the tilee/azure_monitor_eventsource branch January 13, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-recent-activity There has been no recent activity on this issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants