pdreker / fritz_exporter

Prometheus exporter for Fritz!Box home routers
Other
145 stars 32 forks source link

Fix duplicate metrics with multiple devices #180

Closed ein-stein-chen closed 11 months ago

ein-stein-chen commented 1 year ago

The old '_getMetricValues' appended metrics via 'self.metrics[""].add_metric(…)' but then 'returned' all metrics via 'yield self.metrics[""]' for every device.

So when calling '_getMetricValues' for the first device it returns only the metrics for the first device, but when calling it again (for the second device) it returns the metrics for the first and second device (and so forth).

To fix this 'FritzCapability._getMetricValues()' is split into '_generateMetricValues()' and '_getMetricValues()'. With most of the code staying in '_generateMetricValues()' except for the 'yield' statements which are moved into the new '_getMetricValues()'.

With this 'FritzCapability.getMetrics()' can stay largely the same with the generating of the metric values staying in the 'for … devices:' loop, and only the actual retrieving of the metric values moved out of the loop.

pdreker commented 1 year ago

Hello and thanks for your contribution!

Unfortunately this now puts me in a slightly awkward position: I have been working on another branch (https://github.com/pdreker/fritz_exporter/tree/feat/read-data-via-http), which is intended to prepare the code to be able to read some device data via the HTTP API, if it isn't available via TR-064. This is mainly to get more data for cable modem boxes, which don't expose the cable data via the TR-064 API.

This in turn has prompted a rather large refactoring in the code...

In short: fritzcapabilities is not in the new code anymore. The whole "discovery mechanism" has been a pain in the a** and made the code really hard to read (and I wrote it...)

Unfortunately your point still stands: When running against multiple devices it returns metrics twice, if they can be found on both devices.

Obviously the two branches will conflict rather massively, due to the refactoring.

So now there are two options: Either I manually port your changes into my current feature branch or you rework your PR to apply to the branch.

Either way: this needs to be fixed.

pdreker commented 1 year ago

Funnily enough this sounded familiar... https://github.com/pdreker/fritz_exporter/issues/115

Different trigger though...

pdreker commented 11 months ago

Facing the reality of my available time for major rewrites and the ability to fix a major bug by just merging a PR provided by someone else in their free time, I have decided that the pragmatic approach is the best course of action.

In less prosaic words: I have merged your PR to fix the bug and will deal with the implications for my own code at a later stage. ;-)

Thanks again for your contribution! I hope to release a new version today, which will contain your fix.