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

Add Global Error Handler #757

Conversation

MitchellDumovic
Copy link
Contributor

@MitchellDumovic MitchellDumovic commented Aug 4, 2020

Related issue: #696

Make a customizable error handler for non-recoverable errors a "MUST" requirement for all languages. Includes examples from the existing Java and Go implementations of the error handler.

@MitchellDumovic MitchellDumovic requested a review from a team August 4, 2020 20:41
@carlosalberto carlosalberto added area:error-reporting Related to error reporting area:sdk Related to the SDK release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Aug 5, 2020
Co-authored-by: Christian Neumüller <[email protected]>
@carlosalberto
Copy link
Contributor

@Oberon00 Your feedback has been addressed. Looks good to go? ;)

@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review this PR - it's straightforward, and it's essentially a recommendation, rather a strict item, and it's definitely worth having IMHO ;)

Co-authored-by: Christian Neumüller <[email protected]>
@andrewhsu
Copy link
Member

From the bug triage meeting today, looks like this change is good to be merged.

@carlosalberto carlosalberto merged commit e6a4a8d into open-telemetry:master Aug 7, 2020
@bogdandrutu
Copy link
Member

@andrewhsu this is a violation of our public rules, this change was not approved by 2 global approvers. It is a limitation of GitHub that anyone who is approver of one subdirectory counts as approver overall for the 2 required approvals, but our rules is that 2 approvals with ownership which cannot be guaranteed by the current GitHub settings.

Please make sure you enforce our public rules.

@andrewhsu
Copy link
Member

@bogdandrutu apologies for the overstep in the public rules. in the spirit of increasing velocity, this one was targeted for merge at our otel spec issue scrub 🧽 mtg today (i called it bug triage above) given the limited scope and existing 2 approvals.

i admit it is not immediately clear for me to grasp the required minimum approvals for a PR, but now after some digging i found them in another repo here: https://github.com/open-telemetry/community/blob/9b841a653997ec3d708af22006f54b8600a26807/community-members.md#specifications-and-proto

i'll help stick to the public rules in the future.

@carlosalberto
Copy link
Contributor

@bogdandrutu The fault is on me - let's discuss this on the Maintainers meeting next Monday how to proceed here (I'm OOO but happy to jump in for one hour).

For context: we all know approvals are not coming at a fast-enough speed, and that either we have the TC members jump in or we won't have all the important items ready for GA (I remember PRs being stuck because they only got 1 approval and were in the limbo for weeks, for example) - specially for items that are not polemic nor complex.

Also, for reference: this issue was discussed 4 or 5 weeks ago, there was initial overall agreement, then there was some push back by you, then we talked about this 2 weeks ago, and this PR is the result - which, by the way, is more of a suggestion than an enforcement. I'd imagine that by this time it would be clear this is not a polemic issue anymore.

Anyway, happy to discuss this on next Monday in the maintainers meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:error-reporting Related to error reporting area:sdk Related to the SDK release:required-for-ga Must be resolved before GA release, or nice to have before GA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants