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

add config option for InfluxDB field_name #489

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

antonmeyer
Copy link

to allow per vzlogger channel a different field name than "value" we add this config option

@r00t-
Copy link
Contributor

r00t- commented Apr 17, 2021

looks good,
but not tested yet,
wonder if "field_name" is a good idea for the config parameter, as it's influxdb-specific.
(or maybe it can be useful for other APIs too? like for #420 )

@antonmeyer
Copy link
Author

I'm not sure if I got the meaning of the question in the right way. If it is about the name of the parameter, I'm not picky. What ever fits better. Right now it is for influxDB. I can't tell, if it makes sense to #420. As #420 is about UUID i would say no. The main idea is not a kind of UUID, it is more a comfort label for the influxDB query builder.

@r00t-
Copy link
Contributor

r00t- commented Apr 18, 2021

yes, it's about the name,
if it's ONLY used by influxdb, i'd think "influxdb_field_name" or such would be more appropriate.

#420 mentions uuid,
but i think it's really about the issue that mqtt currently does not support ANY channel identifiers, and only numbers the channels in the order they appear in the configuration file.
so mqtt also needs to have a per-channel api-side identifer added to the config, and it might be appropriate to share the config parameter.
but i have not checked in detail.
(that's ofcourse not directly related to this MR, but i think it makes sense to invrstigate before adding a new parameter.)

@antonmeyer
Copy link
Author

"influxdb_field_name" is a long string, but names should be managed by a central instance (you?). So I could live with that name.
I know not much about mgtt to judge if the same idea would improve it.

@r00t-
Copy link
Contributor

r00t- commented Apr 22, 2021

"influxdb_field_name" is a long string

but if we introduced it with the short name, we would have to add comments everywhere,
explaining that it's only for influxdb, to avoid confusing users.
so i think it's better to use the long variant, specifically while it's really only used for influxdb.
if another api used it, the short name might be justified,
but it's still hard to for users to track which options have a meaning for which api.
(a related issue is that "uuid" is mandatory, while only the vz-api uses it, been meaning to fix that.)
consider the config edtior: http://volkszaehler.github.io/vzlogger/
(that also reminds us that the new setting should be added to
https://github.com/volkszaehler/vzlogger/blob/master/etc/vzlogger_generic.schema.json )

maybe @mbehr, @andig, @justinotherguy have an opinion?

@antonmeyer
Copy link
Author

I would prefer the shorter name, and add comments. I'm not the config name expert, but I have the feeling, that several other config parameters are also limited to some domains. the generic schema makes the hierarchy clear. I should add it there, as well as the token. I wonder how to declare alternatives in this schema.
I assume that you always need username / passwd or the token.

@falke9176
Copy link

falke9176 commented Dec 30, 2023

Why hasn't this been implemented yet? This request is open since 2021....
That's exactly what I needed. I added this manually to my InfluxDB files.
This means that multiple entries are possible in one measured value.
This is what it looks like in InlfuxDB:

> show field keys
name: holley
fieldKey fieldType
-------- ---------
1-8-0    float
16-7-0   float
2-8-0    float
31-7-0   float
51-7-0   float
71-7-0   float

The option name could also be called fieldkey, analogous to InfluxDB.
I also prefer the shorter name, as all the others doesn't have "influxdb_" prefix

Thanks a lot !!

@r00t-
Copy link
Contributor

r00t- commented Dec 30, 2023

@falke9176:

This means that multiple entries are possible in one measured value.

logging multiple fields per point would be a great feature to have,
i.e. recording current and power from the same sml/d0 report into a single influxdb record (point),
https://docs.influxdata.com/influxdb/v1/concepts/glossary/#point
but the code will currently always record one point per value,
and this PR does not change that.

interestingly, this says influxdb will automatically merge points with identical timestamps and tags?
https://docs.influxdata.com/influxdb/v1/troubleshooting/frequently-asked-questions/#how-does-influxdb-handle-duplicate-points
so once we have the ability to set the "fieldKey", this could be made use of,
but creating the multi-fields points right in the client should be more efficient.
(and actually logging records with differently structured field sets into the same measurement seems impractical?)

also keep in mind that currently the timestamps are typically not in sync! #455

@falke9176 : do you have code for that already?

@falke9176
Copy link

falke9176 commented Dec 31, 2023

I added/changed the two file as done in this request.
Normally there is only one fieldkey called "value", where all the measurements are wrote to.
No merging of multiple values...... at my InlfuxDB v1.8
Since I added the few lines in the code, now my Influx measurement entry for the holley meter looks like this:

> select * from holley where time > 1703979876705000000
name: holley
time                1-8-0      16-7-0     2-8-0    31-7-0 51-7-0 71-7-0
----                -----      ------     -----    ------ ------ ------
1703979878321000000                                2.49   2.18   1.23
1703979880000000000 28773920.3 972.957606 20910652
1703979880002000000                                2.51   2.19   1.23
1703979881618000000                                2.52   2.18   1.23
1703979883251000000                                2.51   2.17   1.23
1703979884884000000            974        20910652 2.51   2.19   1.23
1703979885000000000 28773921.7 970.64855  20910652
1703979886517000000                                2.5    2.17   1.23
1703979888182000000                                2.51   2.18   1.23
1703979889894000000            973                 2.51   2.18   1.23
1703979891543000000                                              1.23

...it works fine for me... and I can use my grafana dashboards going on....
I came from other languages - f.e. I read the holley with a python script before and the entry were the same scheme

I wonder that this little change isn't official implemented and the request is still open since 2 years.

@J-A-U
Copy link
Collaborator

J-A-U commented Dec 31, 2023

I added/changed the two file as done in this request.

Git has some tools, you don't need to add changes manually:
https://wiki.volkszaehler.org/howto/git#ein_pull-request_koennte_mein_lokales_problem_loesen_wie_kann_ich_ihn_testen

I wonder that this little change isn't official implemented and the request is still open since 2 years.

The PR, so far, is incomplete and has unresolved issues.

See comments like:

but not tested yet,

and

(that also reminds us that the new setting should be added to
https://github.com/volkszaehler/vzlogger/blob/master/etc/vzlogger_generic.schema.json )

@falke9176
Copy link

Git has some tools, you don't need to add changes manually: https://wiki.volkszaehler.org/howto/git#ein_pull-request_koennte_mein_lokales_problem_loesen_wie_kann_ich_ihn_testen

Thanks for the link....;-)

The PR, so far, is incomplete and has unresolved issues.

See comments like:

but not tested yet,

so I can confirm that it works properly for me without any issues

@r00t-
Copy link
Contributor

r00t- commented Jan 3, 2024

quoting @falke9176 :

> select * from holley where time > 1703979876705000000
name: holley
time                1-8-0      16-7-0     2-8-0    31-7-0 51-7-0 71-7-0
----                -----      ------     -----    ------ ------ ------
1703979878321000000                                2.49   2.18   1.23
1703979880000000000 28773920.3 972.957606 20910652
1703979884884000000            974        20910652 2.51   2.19   1.23
1703979885000000000 28773921.7 970.64855  20910652
1703979888182000000                                2.51   2.18   1.23
1703979889894000000            973                 2.51   2.18   1.23
1703979891543000000                                              1.23

imho that is proof that this implementation is very broken,
at least when using the feature to log multiple fields to the same measurement.
it results in points with randomly sparse fields.
this might work for a grafana visualization, but really the data is more or less corrupted.
we should at very least merge #455 (also untested) first.
and ideally add code that assembles the fields into a single point inside vzlogger before sending it to influxdb, instead of relying on influxdb merging the points.

@falke9176
Copy link

imho that is proof that this implementation is very broken,

oh no, it is caused of my vzlogger.config parameters. I thought it would be less data if I aggregate the measurements.
I changed the aggtime to -1 and the aggmode to none for all channels.

now the data looks like this:


> select * from holley where time > '2024-01-03T16:20:00Z'
name: holley
time                1-8-0      16-7-0 2-8-0    31-7-0 51-7-0 71-7-0
----                -----      ------ -----    ------ ------ ------
1704298800176000000 28873199.4 1420   20911520 2.59   3.58   1.53
1704298801808000000 28873200.1 1432   20911520 2.59   3.63   1.53
1704298803441000000 28873200.7 1430   20911520 2.59   3.61   1.53
1704298805106000000 28873201.4 1432   20911520 2.58   3.61   1.53
1704298806883000000 28873202.1 1426   20911520 2.58   3.6    1.53
1704298808516000000 28873202.7 1422   20911520 2.58   3.59   1.53
1704298810180000000 28873203.4 1429   20911520 2.59   3.59   1.53
1704298811813000000 28873204   1427   20911520 2.59   3.61   1.53
1704298813462000000 28873204.7 1429   20911520 2.59   3.61   1.53
1704298815078000000 28873205.3 1438   20911520 2.58   3.63   1.53
1704298816711000000 28873206   1432   20911520 2.58   3.62   1.53
1704298818344000000 28873206.6 1421   20911520 2.58   3.6    1.53
1704298819960000000 28873207.3 1429   20911520 2.58   3.59   1.53
1704298821626000000 28873207.9 1431   20911520 2.59   3.61   1.53
1704298823242000000 28873208.6 1433   20911520 2.59   3.61   1.53
1704298824860000000 28873209.2 1430   20911520 2.59   3.62   1.53
1704298826491000000 28873209.9 1444   20911520 2.59   3.62   1.53
1704298828236000000 28873210.6 1424   20911520 2.58   3.59   1.53
1704298829854000000 28873211.2 1430   20911520 2.59   3.6    1.53
1704298831486000000 28873211.8 1431   20911520 2.58   3.6    1.53
1704298833103000000 28873212.5 1422   20911520 2.58   3.59   1.53
1704298834719000000 28873213.1 1431   20911520 2.58   3.6    1.53
1704298836368000000 28873213.8 1424   20911520 2.58   3.6    1.53
1704298838001000000 28873214.4 1425   20911520 2.59   3.63   1.53
1704298839649000000 28873215.1 1434   20911520 2.59   3.6    1.53
1704298841282000000 28873215.7 1418   20911520 2.59   3.59   1.53
1704298842914000000 28873216.4 1418   20911520 2.59   3.58   1.53
1704298844563000000 28873217   1419   20911520 2.58   3.58   1.53
1704298846196000000 28873217.7 1418   20911520 2.58   3.57   1.53
1704298875489000000 28873229.3 1415   20911520 2.61   3.54   1.53
1704298877138000000 28873229.9 1430   20911520 2.61   3.56   1.53
1704298878754000000 28873230.5 1417   20911520 2.59   3.57   1.53

it's still working for me

@r00t-
Copy link
Contributor

r00t- commented Jan 3, 2024

curious...
which meter type are you using?
as of #426 (to be fixed by the untested/unmerged #655 ), both SML and d0 meters will produce every value with a slightly different timestamp,
yet your data above shows records merged matched at sub-second timestamps.
i wonder if influxdb is even applying some slack when merging the records.
(even then, i'd prefer to merge in vzlogger and not rely on undocumented features.)

@falke9176
Copy link

I'm using the SML type.

It's the old InfluxDB V1.8 and it works....;-)

You only change the field key from “value” to “whateveryouwant”, nothing else in vzlogger.
Alternatively, you can choose a different measurement name for each channel.

The database does the rest internally and would do this for other applications that also write data to it.

So I don't see any problems here right now.

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.

4 participants