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

fix: General err handling for collector #52

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

jneo8
Copy link
Contributor

@jneo8 jneo8 commented Nov 21, 2023

The error handler will try to output the failed metrics and make sure the single collector's bug won't affect other collectors.


Review together with: canonical/hardware-observer-operator#117

@jneo8 jneo8 requested review from a team, Pjack, aieri, agileshaw and rgildein November 21, 2023 09:42
The error handler will try to output the failed metrics and make sure
the single collector's bug won't affect other collectors.
@jneo8 jneo8 force-pushed the fix/general-error-handler-for-collector branch from 11b60ec to fd75a34 Compare November 21, 2023 09:46
@chanchiwai-ray
Copy link
Contributor

chanchiwai-ray commented Nov 22, 2023

Ideally, the collector itself should be responsible to catch general error, and produce a payload when it is failed.

This changes might potentially affect all collectors, can you add functional test? i.e. enable a collector, mock the fetch function to raise exception, and see if it outputs the failed metrics?

Or at least test it manually

@jneo8
Copy link
Contributor Author

jneo8 commented Nov 22, 2023

Ideally, the collector itself should be responsible to catch general error, and produce a payload when it is failed.
This changes might potentially affect all collectors, can you add functional test? i.e. enable a collector, mock the fetch function to raise exception, and see if it outputs the failed metrics?
Or at least test it manually

Yes, unit test and manually test are done.

Agree with your point that collector should responsible to catch it's error. But won't it be the same if we need to have this practice on all the collectors?
Instead of catch general error on each collector, reduce the possible to make human error, like every time you create a new collector and you need to remember this practice, adding catch in core module maybe make more effectually and safe from my point of view.

Copy link

@Pjack Pjack left a comment

Choose a reason for hiding this comment

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

Request to update the test plan and test case.

prometheus_hardware_exporter/core.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/core.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/core.py Show resolved Hide resolved
Copy link
Contributor

@dashmage dashmage left a comment

Choose a reason for hiding this comment

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

A few typos to fix. Otherwise LGTM :)

prometheus_hardware_exporter/core.py Outdated Show resolved Hide resolved
tests/unit/test_collector.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

LGTM thanks

prometheus_hardware_exporter/core.py Show resolved Hide resolved
@jneo8
Copy link
Contributor Author

jneo8 commented Nov 28, 2023

Waiting for canonical/hardware-observer-operator#117

@jneo8 jneo8 merged commit 47b9719 into canonical:main Nov 29, 2023
3 checks passed
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.

6 participants