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 support for MPPT metrics #42

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

sveba
Copy link

@sveba sveba commented Aug 5, 2021

Summary

  • Adds MPPT (Maximum Power Point Tracking) metrics. MPPT metrics are archived by the fronius inverter every 5 mins.
  • This results in an additional API request to the Fronius device with the same --symo.timeout
  • Each API request is done sequentially. That means the total scrape time can be 2x --symo.timeout. Please take that into consideration when configuring Prometheus scrape interval
  • Adds 2 new flags: --symo.enable-power-flow, --symo.enable-archive with default values being true. With these flags, you can also disable each API endpoint.

Breaking Changes

  • The --symo.url flag suggested to use the full URL path like --symo.url http://symo.ip.or.hostname/solar_api/v1/GetPowerFlowRealtimeData.fcgi previously. This has now changed to base host name only, e.g. --symo.url http://symo.ip.or.hostname (without trailing slash)

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    fix, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update documentation.
  • Update tests.

@sveba sveba force-pushed the feature/mppt_data branch 3 times, most recently from cd54c79 to 435e037 Compare August 5, 2021 14:09
@ccremer ccremer added the enhancement New feature or request label Aug 6, 2021
@ccremer
Copy link
Owner

ccremer commented Aug 6, 2021

Hi!
Thanks for the PR. I'll try to review it today or in the weekend.

Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

Good work so far! 🎉
I think it could be even more improved for both user and maintainers though.

README.adoc Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
pkg/fronius/symo_test.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
@sveba sveba force-pushed the feature/mppt_data branch 2 times, most recently from 943eed8 to 3e38203 Compare August 10, 2021 06:17
Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

2 things that came to mind: Right now the timeout option is applied to both requests. Meaning a timeout of 5 seconds could potentially end up in a scrape of 10 seconds. If we have further Data to scrape, 15 seconds etc. It should be at least documented what this means.

However, thinking even further, it's probably a good idea to make the requests in parallel. That should end up in faster scraping. A little more care needs to be given regarding error handling, in order to present only 1 error to the user, I'd propose to add https://github.com/hashicorp/go-multierror into the mix: Each fatal scrape error can be combined into one and it nicely displays multiple occurred errors.

I would be fine with doing this in a second step in a separate PR. If you would be interested in doing this, I would also delay a release until we have parallelization.

Makefile Outdated Show resolved Hide resolved
cfg/types.go Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
pkg/fronius/symo.go Outdated Show resolved Hide resolved
@sveba
Copy link
Author

sveba commented Aug 10, 2021

I would be fine with doing this in a second step in a separate PR. If you would be interested in doing this, I would also delay a release until we have parallelization.

Agree, but additional PR would be my choice

@sveba sveba force-pushed the feature/mppt_data branch 2 times, most recently from 5463458 to 7cda011 Compare August 10, 2021 09:10
Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

Thanks for the work!
Do you know if you are going to work on the parallelization right away or would you like a prerelease first?

@ccremer ccremer added breaking A breaking change for the user and removed enhancement New feature or request labels Aug 10, 2021
@ccremer ccremer changed the title Enhancement MPPT metrics added Add support MPPT metrics Aug 10, 2021
@ccremer ccremer changed the title Add support MPPT metrics Add support for MPPT metrics Aug 10, 2021
@ccremer ccremer merged commit 0dc44f0 into ccremer:master Aug 10, 2021
@sveba
Copy link
Author

sveba commented Aug 10, 2021

Thanks for the work!
Do you know if you are going to work on the parallelization right away or would you like a prerelease first?

I'm leaving in vacation for 1,5 months so I would not be able to work on this for that time. After that no problem.
About release: like I said, this is your baby, you can decide. I have the exporter running on my server so I'm not in a hurry. :)

@ccremer
Copy link
Owner

ccremer commented Aug 10, 2021

Ah ok. I'll wait for a release and try to come up with something, shouldn't be too hard.
Enjoy your vacation and thanks again for your contribution!

@sveba
Copy link
Author

sveba commented Aug 10, 2021

Na klar. Habe ich gerne gemacht. Wie gesagt, bin absoluter Go-Anfänger ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking change for the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants