Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feature request: add a integer lost packets number to the ResponseList object #82

Closed
ilyapashuk opened this issue Jun 25, 2022 · 5 comments · Fixed by #83
Closed

feature request: add a integer lost packets number to the ResponseList object #82

ilyapashuk opened this issue Jun 25, 2022 · 5 comments · Fixed by #83

Comments

@ilyapashuk
Copy link
Contributor

Is your feature request related to a problem? Please describe.
problem: currently ResponseList contains packets_lost variable which contains float ratio of lost packets to all sent packets.
but we can not know how many exactly packets was lost.

Describe the solution you'd like
we may add an other variable to ResponseList object for this and patch "append" method to count the lost packets

Describe alternatives you've considered
we may use math operations to determine lost packets number from the existing data.
for example, if count was 10, and packets_lost is 0.3, we know that 3 packets was lost.
but float operations are not precise.

@alessandromaggio
Copy link
Owner

Hello, and sorry for late reply.
Your suggestion is valid, but I think we should implement it in a different way now that we are at it. We should use the following variables.

  • self.stats_packets_sent = Total packets sent
  • self.stats_packets_returned = Total packet sent for which we have received a reply
  • self.stats_packets_lost = stats_packet_sent - stats_packets_returned (computed)
  • self.stats_success_ratio = stats_packet_returned / stats_packets_sent (computed)
  • self.stats_lost_ratio = 1 - stats_success_ratio (computed)

We can then keep the current name self.packets_lost as a computed property that maps to stats_lost_ratio for backward compatibility. If you edit your PR with those changes we can merge it and implement it in the next minor release.

@ilyapashuk
Copy link
Contributor Author

changes are added, please review them.

@alessandromaggio
Copy link
Owner

Thank you for the code, changes are finem but can you please write tests for them?

@ilyapashuk
Copy link
Contributor Author

tests are changed, please review them

@alessandromaggio
Copy link
Owner

Implemented in Release 1.1.3, out now! Thanks for providing the code for this!

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

Successfully merging a pull request may close this issue.

2 participants