[influxdb] Add option for using metadata value as measurement name #9943

DerOetzi · 2021-01-24T16:15:45Z

Add option for using the metadata value as measurement name. Part of solution for #9556

Signed-off-by: Johannes Ott info@johannes-ott.net

lujop

Thanks for your contribution @DerOetzi
But I see some problems with this because it will break the query part of the addon:

A complete solution should add also itemName as a tag when this option is used and make query part intelligent to use that logic.
Also I would like to include test covering that new functionality.
I added also some minor notes in your code

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml

lujop

Changes explained in previous comment

DerOetzi · 2021-01-25T18:55:18Z

Thanks for your contribution @DerOetzi
But I see some problems with this because it will break the query part of the addon:

A complete solution should add also itemName as a tag when this option is used and make query part intelligent to use that logic.

The itemname is already always included as tag, if I see this correct. But your right I missed the query part. Please review my changes on this.

Also I would like to include test covering that new functionality.

I have added some tests. Please give me feedback, whether they are enough like this.

I added also some minor notes in your code

Renamed the things you commented

...nfluxdb/src/main/java/org/openhab/persistence/influxdb/internal/ItemToStorePointCreator.java

Hilbrand

LGTM. @lujop Can you have a second look at this pull request and approve if ok.

lujop

Thank you for your modifications, I haven't had time to test but it seems ok.
Only two minor things.

....influxdb/src/main/java/org/openhab/persistence/influxdb/internal/InfluxDBMetadataUtils.java

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml

DerOetzi · 2021-02-19T15:11:23Z

I did some refactoring of my code as mentioned above there is no configuration flag for service anymore, but it just checks whether there is a metadata value or not.

But while doing the refactoring I think I have found a bug inside the existing code, of ItemToStorePointCreator which wasn't noticed because of a missing test case. But I'm not sure and need your experience with the code @lujop and @Hilbrand.

At the InfluxFilterCriteriaQueryCreatorImplTest.java there is a testcase testEscapeSimpleItem which test an item with a dot in its name. This one fails now with an IllegalArgumentException when trying to generate MetadataKey from item name.

I have added now such testcase to the ItemToStorePointCreator file as well, which fails although with the old code, without my changes because it tries to access MetadataKey in addPointTags as well.

My question is it a valid item name with a dot, then the question is how to access metadata with such items, or is this testcase invalid in the FilterQuery test?

DerOetzi · 2021-02-19T15:33:07Z

I did some further research from what I see so far an item name cannot contain a dot because this will always fail here:

https://github.com/openhab/openhab-core/blob/029280683087d07d3d52bc1b125e73c17a33917a/bundles/org.openhab.core/src/main/java/org/openhab/core/common/AbstractUID.java#L96

So I think the testcase is not valid and I would remove it?

...xdb/src/test/java/org/openhab/persistence/influxdb/internal/ItemToStorePointCreatorTest.java

DerOetzi · 2021-02-19T18:33:32Z

@lujop and @Hilbrand removed the testcase for item name with dot

lujop

First of all, I apologize for not being able to review the addon before.
Today I've been able to build and test it for InfluxDB 2 and worked correctly.

The code seems good to me also.
I only made a little commit with some minor documentation changes and suggested a change in a condition expanding the negation to make reading the condition clearer. Please accept the commit suggestion.

Good work, and thank you very much for the PR.

...org/openhab/persistence/influxdb/internal/influx1/Influx1FilterCriteriaQueryCreatorImpl.java

lujop

Just remove the unused parameter from config.

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml

Signed-off-by: Johannes Ott <info@johannes-ott.net>

…y builder to find data in correct measurement. Add tests for new feature. Signed-off-by: Johannes Ott <info@johannes-ott.net>

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Signed-off-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

…-INF/config/config.xml Co-authored-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

…nhab/persistence/influxdb/internal/influx1/Influx1FilterCriteriaQueryCreatorImpl.java Co-authored-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

DerOetzi · 2021-02-27T21:47:05Z

First of all, I apologize for not being able to review the addon before.
Today I've been able to build and test it for InfluxDB 2 and worked correctly.

The code seems good to me also.
I only made a little commit with some minor documentation changes and suggested a change in a condition expanding the negation to make reading the condition clearer. Please accept the commit suggestion.

Good work, and thank you very much for the PR.

Thanks for your constructive review comments. Think we found a good solution together now.

Signed-off-by: Johannes Ott <info@johannes-ott.net>

DerOetzi · 2021-02-27T22:31:11Z

Please don't merge yet I think I have found another problem on restoring item states

Signed-off-by: Johannes Ott <info@johannes-ott.net>

DerOetzi · 2021-02-28T00:08:41Z

Please don't merge yet I think I have found another problem on restoring item states

@lujop can you please recheck my last changes to V1 Query parts. As I noticed there was a bug because tem-tags where not accquired.

lujop · 2021-02-28T10:29:10Z

Please don't merge yet I think I have found another problem on restoring item states

@lujop can you please recheck my last changes to V1 Query parts. As I noticed there was a bug because tem-tags where not accquired.

Sure, let me find some time and I will install InfluxDB1 and do some more detailed testing

DerOetzi · 2021-02-28T23:16:10Z

Please don't merge yet I think I have found another problem on restoring item states

@lujop can you please recheck my last changes to V1 Query parts. As I noticed there was a bug because tem-tags where not accquired.

Sure, let me find some time and I will install InfluxDB1 and do some more detailed testing

I have installed a v1.8 docker container and did some more testing myself. For me everything seems to work fine now, but I would be very glad, if find the time to check it as well @lujop

lujop

I've done some testing with InfluxDB1 and it seems to be working correctly.
Thanks for the fixs.

lujop · 2021-03-05T18:52:15Z

@Hilbrand It seems ok to be integrated for me.

DerOetzi · 2021-03-21T12:58:55Z

@Hilbrand and @lujop is there any problem with this PR? Why isn't it merged yet?

kaikreuzer · 2021-03-21T15:47:48Z

As @Hilbrand already approved on Feb 3, I'd say we can merge. 👍

…penhab#9943) * Add option for using metadata value as measurement name Also-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net> Signed-off-by: John Marshall <john.marshall.au@gmail.com>

…penhab#9943) * Add option for using metadata value as measurement name Also-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

DerOetzi requested a review from lujop as a code owner January 24, 2021 16:15

lujop reviewed Jan 24, 2021

View reviewed changes

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved

lujop requested changes Jan 24, 2021

View reviewed changes

DerOetzi requested a review from lujop January 27, 2021 15:55

Hilbrand requested changes Feb 3, 2021

View reviewed changes

...nfluxdb/src/main/java/org/openhab/persistence/influxdb/internal/ItemToStorePointCreator.java Outdated Show resolved Hide resolved

DerOetzi requested a review from Hilbrand February 3, 2021 13:14

Hilbrand approved these changes Feb 3, 2021

View reviewed changes

lujop requested changes Feb 3, 2021

View reviewed changes

....influxdb/src/main/java/org/openhab/persistence/influxdb/internal/InfluxDBMetadataUtils.java Outdated Show resolved Hide resolved

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved

DerOetzi requested review from Hilbrand and lujop February 19, 2021 15:25

DerOetzi commented Feb 19, 2021

View reviewed changes

...xdb/src/test/java/org/openhab/persistence/influxdb/internal/ItemToStorePointCreatorTest.java Outdated Show resolved Hide resolved

lujop approved these changes Feb 26, 2021

View reviewed changes

...org/openhab/persistence/influxdb/internal/influx1/Influx1FilterCriteriaQueryCreatorImpl.java Outdated Show resolved Hide resolved

lujop requested changes Feb 27, 2021

View reviewed changes

bundles/org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml Outdated Show resolved Hide resolved

Johannes Ott and others added 13 commits February 27, 2021 22:41

Add option for using metadata value as measurement name

ac41782

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Rename new configuration param to useMetaMeasurementName. Modify quer…

2558e0e

…y builder to find data in correct measurement. Add tests for new feature. Signed-off-by: Johannes Ott <info@johannes-ott.net>

Rename configuration parameter

15865b5

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Refactor calculation of Metadata name method

a13790e

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Moved configuration isReplaceUnderscore back to original place

76d7102

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Removed unneccessary Nullable annotation

b604391

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Call calculation method only if configuration enabled

86c2f75

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Refactored without configuration Test failure

4c04d7d

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Added failing Test for sample.Item

63aebb9

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Removed wrong testcase and review comment in README

70f4a93

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Minor documentation changes

15038f3

Signed-off-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

Update bundles/org.openhab.persistence.influxdb/src/main/resources/OH…

c2e0130

…-INF/config/config.xml Co-authored-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

Update bundles/org.openhab.persistence.influxdb/src/main/java/org/ope…

46c8906

…nhab/persistence/influxdb/internal/influx1/Influx1FilterCriteriaQueryCreatorImpl.java Co-authored-by: Joan Pujol <joanpujol@gmail.com> Signed-off-by: Johannes Ott <info@johannes-ott.net>

Johannes Ott added 2 commits February 27, 2021 23:10

Syntax error in review suggestion

a1ee720

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Small typo fixed

c9fa173

Signed-off-by: Johannes Ott <info@johannes-ott.net>

Fixed bug in V1 query

16a2d25

Signed-off-by: Johannes Ott <info@johannes-ott.net>

lujop approved these changes Mar 5, 2021

View reviewed changes

kaikreuzer merged commit cdd99c9 into openhab:main Mar 21, 2021

kaikreuzer added the enhancement An enhancement or new feature for an existing add-on label Mar 21, 2021

kaikreuzer added this to the 3.1 milestone Mar 21, 2021

cweitkamp mentioned this pull request Apr 9, 2021

InfluxDB Persistence not working anymore in 3.1.0-M3 build #10464

Closed

J-N-K mentioned this pull request Apr 17, 2021

[influxdb] allow defining measurement name in item metadata and code improvements smarthomej/addons#71

Merged

brevilo mentioned this pull request Apr 26, 2021

[influxdb] Unable to set _measurement name by metadata using InfluxDb 2.0 #10507

Closed

lujop mentioned this pull request May 12, 2021

[influxdb] Fix issue #10464 Problem querying old historical data #10680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[influxdb] Add option for using metadata value as measurement name #9943

[influxdb] Add option for using metadata value as measurement name #9943

DerOetzi commented Jan 24, 2021

lujop left a comment

lujop left a comment

DerOetzi commented Jan 25, 2021

Hilbrand left a comment

lujop left a comment

DerOetzi commented Feb 19, 2021

DerOetzi commented Feb 19, 2021

DerOetzi commented Feb 19, 2021

lujop left a comment •

edited

Loading

lujop left a comment

DerOetzi commented Feb 27, 2021

DerOetzi commented Feb 27, 2021

DerOetzi commented Feb 28, 2021

lujop commented Feb 28, 2021

DerOetzi commented Feb 28, 2021

lujop left a comment

lujop commented Mar 5, 2021

DerOetzi commented Mar 21, 2021

kaikreuzer commented Mar 21, 2021

[influxdb] Add option for using metadata value as measurement name #9943

[influxdb] Add option for using metadata value as measurement name #9943

Conversation

DerOetzi commented Jan 24, 2021

lujop left a comment

Choose a reason for hiding this comment

lujop left a comment

Choose a reason for hiding this comment

DerOetzi commented Jan 25, 2021

Hilbrand left a comment

Choose a reason for hiding this comment

lujop left a comment

Choose a reason for hiding this comment

DerOetzi commented Feb 19, 2021

DerOetzi commented Feb 19, 2021

DerOetzi commented Feb 19, 2021

lujop left a comment • edited Loading

Choose a reason for hiding this comment

lujop left a comment

Choose a reason for hiding this comment

DerOetzi commented Feb 27, 2021

DerOetzi commented Feb 27, 2021

DerOetzi commented Feb 28, 2021

lujop commented Feb 28, 2021

DerOetzi commented Feb 28, 2021

lujop left a comment

Choose a reason for hiding this comment

lujop commented Mar 5, 2021

DerOetzi commented Mar 21, 2021

kaikreuzer commented Mar 21, 2021

lujop left a comment •

edited

Loading