Update InfluxDB to handle datetime objects and multiple decimal points #8080
Conversation
Updates the InfluxDB regex to ignore datetime objects being coverted into float values. Adds tests to the component to ensure datetime objects are corectly handled.
|
@philhawthorne, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @titilambert and @kk7ds to be potential reviewers. |
tests/components/test_influxdb.py
Outdated
| @@ -164,7 +167,8 @@ def test_event_listener(self, mock_client): | |||
| 'battery_level': 99.0, | |||
| 'temperature_str': '20c', | |||
| 'temperature': 20.0, | |||
| 'last_seen_str': 'Last seen 23 minutes ago' | |||
| 'last_seen_str': 'Last seen 23 minutes ago', | |||
| 'updated_at_str': '2017-01-01 00:00:00' | |||
homeassistant/components/influxdb.py
Outdated
| @@ -165,7 +167,8 @@ def influx_event_listener(event): | |||
| except (ValueError, TypeError): | |||
| new_key = "{}_str".format(key) | |||
| json_body[0]['fields'][new_key] = str(value) | |||
| if non_digit_tail.match(json_body[0]['fields'][new_key]): | |||
| if non_digit_tail.match(json_body[0]['fields'][new_key]) and not isinstance( | |||
There was a problem hiding this comment.
line too long (96 > 79 characters)
Fixes errors from Hound bot
homeassistant/components/influxdb.py
Outdated
| @@ -165,7 +167,9 @@ def influx_event_listener(event): | |||
| except (ValueError, TypeError): | |||
| new_key = "{}_str".format(key) | |||
| json_body[0]['fields'][new_key] = str(value) | |||
| if non_digit_tail.match(json_body[0]['fields'][new_key]): | |||
| if non_digit_tail.match( | |||
| json_body[0]['fields'][new_key]) and not isinstance( | |||
There was a problem hiding this comment.
line too long (80 > 79 characters)
Changes the way InfluxDB handles values such as 1.2.3.4 to be 1.234 so it stores in InfluxDB as a valid float value
homeassistant/components/influxdb.py
Outdated
| json_body[0]['fields'][key] = float( | ||
| non_decimal.sub('', value)) | ||
| if non_digit_tail.match( | ||
| json_body[0]['fields'][new_key]) and not isinstance( |
There was a problem hiding this comment.
line too long (80 > 79 characters)
homeassistant/components/influxdb.py
Outdated
| json_body[0]['fields'][key] = float( | ||
| non_decimal.sub('', value)) | ||
| if non_digit_tail.match( | ||
| json_body[0]['fields'][new_key]) and not isinstance( |
There was a problem hiding this comment.
line too long (80 > 79 characters)
Reduce the size of a line for the linter
|
2017-06-18 12:01:42 ERROR (Recorder) [homeassistant.core] Error doing job: Future exception was never retrieved
Traceback (most recent call last):
File "/home/homeassistant/.homeassistant/custom_components/myinfluxdb.py", line 170, in influx_event_listener
json_body[0]['fields'][key] = float(value)
ValueError: could not convert string to float: '8.5,8.1,9.6,8.9,9.6,9.3,3.7'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/homeassistant/.homeassistant/custom_components/myinfluxdb.py", line 179, in influx_event_listener
non_decimal.sub('', value))
ValueError: could not convert string to float: '8.58.19.68.99.69.33.7'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/homeassistant/.pyenv/versions/3.5.2/lib/python3.5/concurrent/futures/thread.py", line 55, in run
result = self.fn(*self.args, **self.kwargs)
File "/home/homeassistant/.homeassistant/custom_components/myinfluxdb.py", line 182, in influx_event_listener
re.sub(r'^([^.]*\.)(.*)$', decreplace, value))
ValueError: could not convert string to float: '8.5,81,96,89,96,93,37'I'm thinking... what need is there for conversion if the string is already being stored? Would not it be better, simply, to give up conversion in such cases? except (ValueError, TypeError):
pass |
|
Hey @azogue So the original intention for this was to catch any components that are sending numeric data, but as strings. For example, "45%" instead of "45". When this happens, you can't use the string value in Grafana or other graph suites, as the data stored in InfluxDB isn't a numerical value. This change detects values such as "45%", strips out the "%" character and leaves two values, "45" and a _str of "45%". This way, both values are stored in InfluxDB and can be used.< 8000 /p> The alternative is each time a new platform comes along, if the developer doesn't return a numerical value for say the battery level, we'll then have to go back and update that component to return the correct data. |
|
hi @philhawthorne, I understand the initial intention, and it seems phenomenal to me, but, for the cases of more complex strings with numbers, perhaps the best alternative is to avoid the parsing, right? For cases of 'number + unit' I think the change is great. |
homeassistant/components/influxdb.py
Outdated
| json_body[0]['fields'][key] = float( | ||
| non_decimal.sub('', value)) | ||
| except (ValueError, TypeError): | ||
| json_body[0]['fields'][key] = float( |
There was a problem hiding this comment.
@philhawthorne, sorry if I was not understood, I was referring to this except clause, to simply pass if a ValueError is raised.
If we get an error trying to convert a variable to a float, let's ignore it completely
|
Thanks @azogue That makes more sense. Makes it more simple, and handles both traces that were reported in the issue. I've updated that as you suggested. Cheers! |
|
Just wanted to confirm that this change fixes the original issue I reported in #8042. |
There was a problem hiding this comment.
https://github.com/philhawthorne/home-assistant/blob/74ea240dc05d2d3e1fe6161146f7d0e168946a19/homeassistant/components/influxdb.py#L152-L153 should be as constante on top of component to save compile time every call.
Makes the Regex's used by InfluxDB constants so they don't need to be compiled each time
|
Thanks @pvizeli changed that |
|
I think instead to create this complex things we should use match regex: |
|
I update the regex for better detection |
tests/components/test_influxdb.py
Outdated
| 'last_seen': 23.0, | ||
| 'updated_at_str': '2017-01-01 00:00:00', | ||
| 'updated_at': 20170101000000, | ||
| 'multi_periods_str': '0.120.240.2023873' }, |
There was a problem hiding this comment.
line too long (86 > 79 characters)
whitespace before '}'
|
Thanks @pvizeli ! |
|
Good teamwork! |
#8080) * Update InfluxDB to handle datetime objects Updates the InfluxDB regex to ignore datetime objects being coverted into float values. Adds tests to the component to ensure datetime objects are corectly handled. * Fix Hound errors Fixes errors from Hound bot * Update InfluxDB to handle multiple decimal points Changes the way InfluxDB handles values such as 1.2.3.4 to be 1.234 so it stores in InfluxDB as a valid float value * Fix lint issues Reduce the size of a line for the linter * Update InfluxDB to pass on unknown variable If we get an error trying to convert a variable to a float, let's ignore it completely * Make InfluxDB Regex constants Makes the Regex's used by InfluxDB constants so they don't need to be compiled each time * cleanup * fix lint * Update regex * fix tests * Fix JSON body missing new line character * fix exceptions
|
Cherry-picked for 0.47.1 |
home-assistant#8080) * Update InfluxDB to handle datetime objects Updates the InfluxDB regex to ignore datetime objects being coverted into float values. Adds tests to the component to ensure datetime objects are corectly handled. * Fix Hound errors Fixes errors from Hound bot * Update InfluxDB to handle multiple decimal points Changes the way InfluxDB handles values such as 1.2.3.4 to be 1.234 so it stores in InfluxDB as a valid float value * Fix lint issues Reduce the size of a line for the linter * Update InfluxDB to pass on unknown variable If we get an error trying to convert a variable to a float, let's ignore it completely * Make InfluxDB Regex constants Makes the Regex's used by InfluxDB constants so they don't need to be compiled each time * cleanup * fix lint * Update regex * fix tests * Fix JSON body missing new line character * fix exceptions
Description:
Updates the InfluxDB regex to ignore datetime objects being coverted
into float values.
Adds tests to the component to ensure datetime objects are corectly
handled.
Related issue (if applicable): fixes #8042
Checklist:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass