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

sensors module fix #4651

Merged
merged 3 commits into from Nov 13, 2018
Merged

sensors module fix #4651

merged 3 commits into from Nov 13, 2018

Conversation

ilyam8
Copy link
Member

@ilyam8 ilyam8 commented Nov 13, 2018

Summary

Fix: #4602

Component Name

python module sensors

Additional Information

bug trace:

2018-11-12 21:25:49: python.d ERROR: sensors: sensors: check() unhandled exception: Can't read
2018-11-12 21:25:49: python.d ERROR: sensors: sensors: Traceback (most recent call last):
File "./plugins.d/python.d.plugin", line 317, in check_job
check_ok = bool(job.check())
File "/opt/netdata/usr/libexec/netdata/python.d/sensors.chart.py", line 144, in check
self.create_definitions()
File "/opt/netdata/usr/libexec/netdata/python.d/sensors.chart.py", line 120, in create_definitions
vals = [sensors.get_value(chip, sf.number) for sf in sfi]
File "/opt/netdata/usr/libexec/netdata/python.d/python_modules/third_party/lm_sensors.py", line 181, in get_value
raise Exception(strerror(err))
Exception: Can't read

quick fix:
skip unreadable values


@hznovich please test the fix

Module is small but extremely hard to read.

@ilyam8 ilyam8 requested a review from Ferroin November 13, 2018 18:21
@Ferroin
Copy link
Member

Ferroin commented Nov 13, 2018

Wait, so the lm_sensors module actually raises a bare Exception if it fails a read?

If so, that should be reported upstream as a bug (or we should update if it's fixed upstream), nothing should ever raise Exception directly.

@ilyam8
Copy link
Member Author

ilyam8 commented Nov 13, 2018

actually raises a bare Exception if it fails a read?

it raises nothing except bare Exception actually

netdata uses this

@ilyam8
Copy link
Member Author

ilyam8 commented Nov 13, 2018

lm_senors
readme, Missing Features (pull requests are welcome) section:

  • Error handlers

@Ferroin 😄

@Ferroin
Copy link
Member

Ferroin commented Nov 13, 2018

I guess I'll be submitting a PR to that then to add a local Exception sub-class for it to throw...

Short term, this is fine, but we really should not be catching a bare exception and blindly ignoring it.

@Ferroin
Copy link
Member

Ferroin commented Nov 13, 2018

PR created upstream to add something saner for us to catch: paroj/sensors.py#2

Hopefully the author is still around, it's been 8 months since the last commit.

@ilyam8
Copy link
Member Author

ilyam8 commented Nov 13, 2018

Author is very active on github, i am sure it will respond soon.

How about to mirror C code errors btw?

like

this is our get_value

error codes

Implementation:

ERR_WILDCARDS = 1
ERR_NO_ENTRY = 2

class SensorsError(Exception):
    pass

class ErrorWildcards(SensorsError):
    pass

class ErrorNoEntry(SensorsError):
    pass

...
def is_error(idx):
    return idx < 0


def error_factory(idx):
    idx = abs(idx)

    if idx == ERR_WILDCARDS:
        return ErrorWildcards
    elif idx == ERR_NO_ENTRY:
        return ErrorNoEntry


...
    err = _hdl.sensors_parse_chip_name(orig_name.encode("utf-8"), byref(ret))
    
    if is_error(err):
        raise  error_factory(err)(strerror(err))

?

@Ferroin
Copy link
Member

Ferroin commented Nov 13, 2018

I was just trying for something quick to make sure it was usable. I'll probably post an update to it to mirror C errors, but that will probably have to wait until tomorrow.

@ktsaou
Copy link
Member

ktsaou commented Nov 13, 2018

merge?

@ilyam8 ilyam8 merged commit d0cc791 into netdata:master Nov 13, 2018
@ilyam8 ilyam8 mentioned this pull request Nov 13, 2018
@ilyam8
Copy link
Member Author

ilyam8 commented Nov 14, 2018

@Ferroin upstream is fixed, do you have time to update netdata's lm_sensors and sensors module?

@Ferroin
Copy link
Member

Ferroin commented Nov 15, 2018

Yes, I'll have a PR to do so later today.

@ilyam8 ilyam8 deleted the sensors_fix branch November 28, 2018 17:29
@paulfantom paulfantom added area/external/python area/collectors Everything related to data collection labels Nov 29, 2018
kiku-jw pushed a commit to kiku-jw/netdata that referenced this pull request Mar 4, 2019
* sensors: survive sensors.get_value Exception

* sensors: log unreadable subfeature in check

* sensors: add todos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collectors Everything related to data collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sensors don`t work
4 participants