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

FRITZ!Box: use 64-bit traffic counters if available #739

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbunkus
Copy link

@mbunkus mbunkus commented Aug 5, 2024

The current FRITZ!Box check uses traffic counters that are 32-bit wide. This means that they overflow quickly, causing gaps in graphs & wrong traffic stats in general. Most current FRITZ!Box models also send additional 64-bit counters. These can & should be used if they're present, which is what this fix implements. If they're not available, the old 32-bit counters are used.

Copy link

github-actions bot commented Aug 5, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@mbunkus
Copy link
Author

mbunkus commented Aug 5, 2024

I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA.

@mbunkus
Copy link
Author

mbunkus commented Aug 5, 2024

recheck

@Yogibaer75
Copy link
Contributor

I think this change will give an error with the tests.
The changed line should look like this.

            interfaces.Counters(
                in_octets=int(
                    section.get(
                        "NewX_AVM_DE_TotalBytesReceived64", section.get("NewTotalBytesReceived", 0)
                    )
                ),
                out_octets=int(
                    section.get("NewX_AVM_DE_TotalBytesSent64", section.get("NewTotalBytesSent", 0))
                ),
            ),

It is not a syntax problem only formatting^^
All the tests are running as expected.
But these tests should later than also be extended to reflect the new counters.

@mbunkus
Copy link
Author

mbunkus commented Aug 6, 2024

Right, I hadn't looked into tests at all. Honestly this was just supposed to be a reminder to the devs to fix the issue upstream & an idea how to fix them. After unsuccessfully fighting with the CLA bot here I just cannot be bothered looking into having to set up tests, too.

Copy link

This PR is stale because it has been open for 14 days with no activity and the Github Actions are not passing.

@github-actions github-actions bot added the Stale Scheduled for auto-close label Aug 21, 2024
Copy link

github-actions bot commented Sep 4, 2024

This PR was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this Sep 4, 2024
@martinhv
Copy link
Member

I asked my colleague to take a look at the CLA bot

@martinhv martinhv reopened this Sep 16, 2024
@github-actions github-actions bot removed the Stale Scheduled for auto-close label Sep 17, 2024
@englertor
Copy link
Contributor

I'm sorry that you are struggling with the CLA bot. So far I can see nothing suspicious in the execution.

Most likely it is that your comment is not literally the required text, but also contains the "recheck". It should work by only including

I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA.

in a single comment.

@mbunkus
Copy link
Author

mbunkus commented Sep 18, 2024

The bot literally contained the following in the same reply:

You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Its comment is entirely confusing. If the intent is for the user to post a comment solely consisting of "I have read" without the "recheck", why the additional comment about the "recheck"? 'cause that would imply that it does check each and every comment. Is the "recheck" meant solely for when you edit a comment? Or should I post two comments, one with "I have read…" and one with "recheck"? If not, why is the "recheck" comment there in the first place!?

@mbunkus
Copy link
Author

mbunkus commented Sep 18, 2024

recheck

1 similar comment
@englertor
Copy link
Contributor

recheck

@Yogibaer75
Copy link
Contributor

And it's not working ;)

@englertor
Copy link
Contributor

@Yogibaer75 well, the comment of the CLA bot updated and we have the necessary entry in the CLA registry.

Still don't know, why the GH actions didn't pass, but I'm looking into it.

@englertor
Copy link
Contributor

Thanks, @mbunkus for jumping through the hoops to make the CLA bot happy.

Regarding the still failing actions, I've found in the GH logs that it is an issue with how they setup their tests:

HttpError: Unable to retry this workflow run because it was created over a month ago

Apparently, it's not possible to re-run tests that are older than one month as they state in their docs:
https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/re-running-workflows-and-jobs

The preferred approach that Github themselves suggest is either a) adding an empty dummy commit or b) if the upstream changes, rebasing on the latest commit.

Given that information, to make the actions run again in this PR you would need to rebase the PR on latest master.

I understand that this is completely bogus.
Therefore I've added the "tracked" label (to prevent the PR going stale) and created an internal ticket to review your changes regardless of the failing tests.

Thank you for your contribution.

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

Successfully merging this pull request may close these issues.

4 participants