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

[CT-400] Event module should handle "warn on error" behavior. #4914

Closed
nathaniel-may opened this issue Mar 21, 2022 · 3 comments
Closed

[CT-400] Event module should handle "warn on error" behavior. #4914

nathaniel-may opened this issue Mar 21, 2022 · 3 comments
Labels
logging tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@nathaniel-may
Copy link
Contributor

nathaniel-may commented Mar 21, 2022

There are some places in the code that conditionally fire otherwise identical warning events and error events. For example see these lines in the printer task. Because the logged message is identical in both of these events, they should not be two separate events.

The proposed solution:

  1. respect the flag --warn-error within the event module
  2. add a default named parameter to fire_event for adhoc error-on-warn behavior. (i.e. - error_on_warn=False)
  3. for all events that have a warning and error variant remove the error variant and use the added flag to fire_event at the call sites.
@nathaniel-may nathaniel-may added the tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality label Mar 21, 2022
@github-actions github-actions bot changed the title Event module should handle "warn on error" behavior. [CT-400] Event module should handle "warn on error" behavior. Mar 21, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 21, 2022

This is eerily similar to (but indeed separate from) our warn_or_error behavior. That's also something we should look to rework, when we bring exceptions into the same structured framework: #4836 (comment)

@jtcohen6
Copy link
Contributor

Realized that #5424 was another instance of this tech debt (and an actual bug, since it's not working correctly today)

@stu-k
Copy link
Contributor

stu-k commented Sep 8, 2022

Having looked through the event types in events/types.py and where they are emitted, I believe the only events with identical error/warning variants are as follows:

Additionally, it would seem that our events are defined as whatever level they are (i.e. test, debug, info, warn, error) via inheritance, so emitting one event as both warn and error would require a refactor of how our events are defined.

Given this information, I'm not sure it makes sense to have the fire_event function optionally turn events from warnings into errors given it would introduce unnecessary complexity into the events to be able to be transformed from one type into another.

I will close this for now in favor of #5424. Please feel free to reopen should my assessment be incorrect.

@stu-k stu-k closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
Development

No branches or pull requests

3 participants