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

[Proof on Concept] Add initial support to export metrics for Prometheus #7216

Closed
wants to merge 6 commits into from

Conversation

marius
Copy link

@marius marius commented Dec 14, 2019

Description:

See https://prometheus.io/ for project information.

Checklist:

  • The pull request is done against the latest dev branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR.
  • The code change is tested and works on core 2.6.1
  • The code change pass travis tests. Your PR cannot be merged unless tests pass
  • I accept the CLA.

@marius marius marked this pull request as ready for review December 14, 2019 14:13
@marius
Copy link
Author

marius commented Dec 14, 2019

Hi everybody,

this is my first PR for embedded software. I have no idea what I'm doing.
Please review and let me know if this is something people are generally
interested in.

I don't know how the process works to add translations, so I added
English "dummy" entries for all languages.

Regarding the checklist: I don't know what core 2.6.1 is.

Copy link
Collaborator

@s-hadinger s-hadinger left a comment

Choose a reason for hiding this comment

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

Please be frugal in flash size. Remove all help text and keep it utra-minimal.

@Jason2866
Copy link
Collaborator

Thx for the PR, but adding probitary enhancements have to be well considered.
How many users of Tasmota do have a benefit of this?
What is in general the benefit?

@ascillato
Copy link
Contributor

It is missing a define key in order to disable this and not adding more flash usage if this feature is not needed

@marius
Copy link
Author

marius commented Dec 14, 2019

@s-hadinger: Sure, will do.

@Jason2866: The Prometheus protocol is not proprietary, and it is not a small project either.
As usual in open source, there is very little information about the number of users. I'd say it
is one of the most used monitoring systems available.

@ascillato: Yes, will do.

General question: I don't understand the code structure, esp. how .ino files work.
Is there a way for me to access the Energy struct defined in tasmota/xdrv_03_energy.ino?

@jziolkowski
Copy link
Contributor

jziolkowski commented Dec 14, 2019

I agree with @Jason2866 re: gains for users. Prometheus is a pull-based metrics collector, which needs code like this to continuously push data to prometheus. The same result can be obtained by simply configuring influxdb (which using the prometheus authors own words is better for event handling) with telegram's mqtt_consumer, without a single byte of code added to Tasmota.

@marius
Copy link
Author

marius commented Dec 14, 2019

jziolkowski: The thing is, I have Prometheus running, and I'm not interested in setting up Influxdb.
I could create/use an MQTT to Prometheus exporter, but that adds another component to my setup,
whereas this supports the usual way how Prometheus polls devices directly.

@s-hadinger
Copy link
Collaborator

Can you measure the code increase with your PR. I'm surprised that %f works in printf, float is normally disabled to save a lot of code.

@marius
Copy link
Author

marius commented Dec 14, 2019

Actually, it doesn't work, I need to rework that too.
I'll measure the difference once I have the #defines.

@ascillato2
Copy link
Collaborator

@marius

Hi, any news on this PR?

@ascillato2 ascillato2 added the awaiting feedback Action - Waiting for response or more information label Dec 19, 2019
@marius
Copy link
Author

marius commented Dec 19, 2019

Hi Adrian,

sorry I didn't have time to work on this.
Can I change this PR back to draft?

@ascillato
Copy link
Contributor

No need for that. We wait for you.

@marius marius reopened this Dec 21, 2019
@marius
Copy link
Author

marius commented Dec 21, 2019

I added #defines and switched from printing floats to printing unsigned integers.

I would like to access the Energy struct, but I can't because it is defined in a later .ino file.
What do people think about creating an energy.h header file and moving the struct definition
and variable there? That way I can access it in xdrv_01_webserver.ino.

@arendst
Copy link
Owner

arendst commented Dec 21, 2019

Don't change any core items. Energy is conditional anyway so it might not veen be there in some compiles.

I'll scan over your implementation and see how it currently integrates.

@ascillato2 ascillato2 removed the awaiting feedback Action - Waiting for response or more information label Dec 22, 2019
arendst added a commit that referenced this pull request Jan 1, 2020
Add optional support for Prometheus using file xsns_91_prometheus.ino (#7216)
@arendst
Copy link
Owner

arendst commented Jan 1, 2020

Implemented basic prometheus support using single file xsns_91_prometheus.ino

As long as define USE_PROMETHEUS is disabled nothing is reported.

@arendst arendst closed this Jan 1, 2020
@lomik
Copy link

lomik commented Jan 10, 2020

Prometheus can't fetch metrics with error invalid metric type "guage"

My current metrics:

# TYPE global_temperature gauge
global_temperature 43.1
# TYPE voltage gauge
voltage 0
# TYPE current gauge
current 0.000
# TYPE active_power guage
active_power 0
# TYPE energy_daily gauge
energy_daily 0.000
# TYPE energy_total counter
energy_total 0.000

And it will be nice to add prefix "tasmota_" to all metric names

@joba-1
Copy link
Contributor

joba-1 commented Jan 10, 2020

gauge, not guage for active_power?

arendst added a commit that referenced this pull request Jan 11, 2020
Fix typo in gauge metric type (#7216)
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.

9 participants