Skip to content
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

Updated custom Event Hub exceptions #151

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

kyclai
Copy link
Contributor

@kyclai kyclai commented Dec 14, 2021

Updated custom Event Hub configuration exceptions:

@kyclai kyclai requested a review from a team as a code owner December 14, 2021 21:22

public static void SetConnectorOperation(string connectorOperation)
public static string ConnectorOperation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Just curious - any reason to change to property?
Personally speaking, its not intuitive that properties are one time set so its easy to mistake this as an updateable one until we hit a runtime exception. But that's just my thinking.
Or maybe consider adding a comment indicating this is a one time set property so that it shows in intellisense, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In commit 65a6c34, since I needed a get accessor for the operation to specify the operation as Normalization/FhirTransformation, I thought I'd convert SetConnectorOperation to a set accessor to pair the accessors up under one ConnectorOperation property.

I reverted commit 65a6c34 in commit 2a6520f (see other comment), so this change got reverted, but I'll keep your point in mind in the future, thanks!

: base(
message,
innerException,
name: $"{_errorType}{errorName}",
operation: ConnectorOperation.Setup)
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually preferred these exceptions being categorized as Setup as opposed to Normalization/FhirTransformation to differentiate from actual operation issues post setup. Just curious on why we want to change this?
Adding @wi-y to add any thoughts as some of the earlier related metrics were also intentionally set with Setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember we had discussed categorizing the Operation as Normalization/FhirTransformation to identify which Event Hub is the improperly configured one, but maybe I misunderstood which scenario to specify the Operation.

I checked with @wi-y offline, and keeping the Operation as Setup is more preferable and consistent with how we've been classifying Operation in the error metrics so far. Also, as you pointed out, the pod name can already help identify the Event Hub.

Therefore I reverted this change in commit 2a6520f.

@kyclai kyclai merged commit ad94618 into master Jan 19, 2022
@kyclai kyclai deleted the personal/clai/event-exceptions-update branch January 19, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants