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

'Flatten' graph spikes #73

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

Conversation

excurso
Copy link

@excurso excurso commented Jul 20, 2024

Fixed the issue with bandwidth graphs showing spikes at startup, which, because of the high value of the spike, made the rest of the values invisible for some time. Now the average value of the adjacent values are replacing the spike value, if the spike value is greater than the bandwidth burst value.

Fixed the issue with bandwidth graphs showing spikes at startup, which,
because of the high value of the spike, made the rest of the values
invisible for some time. Now the average value of the adjacent values
are replacing the spike value, if the spike value is greater than
the bandwidth burst value.
@atagar
Copy link
Collaborator

atagar commented Jul 28, 2024

Hi excurso, thank you for sending this patch!

I'm delighted that this works for you but I don't think that we should apply this to the main repository. Fabricating data is a bit of a hack. ;P

The proper fix for this would involve figure out why the spikes occur.

@atagar atagar closed this Jul 28, 2024
@excurso excurso mentioned this pull request Jul 29, 2024
@excurso
Copy link
Author

excurso commented Jul 29, 2024

@atagar: Why are you closing the PR without any discussion?

No, it is not a hack. A client program should verify the data it receives before processing it further. This currently doesn't happen in the master branch of nyx.

What do you think is more accurate? An average of adjacent values or spikes of several GB/s, where most people do not even have such much bandwidth?

Look at the issue #31524. Nobody was able to solve this on Tor's side for 4 years. It is labeled with "Mystery Bug". Do you expect a change soon? So let the issue of tor be an issue of nyx for another 10 years, hm?
Why not take over "the hack", at least as long as the issue on Tor's side is not resolved? It can still be removed later.

@atagar
Copy link
Collaborator

atagar commented Jul 29, 2024

Hi excurso. I left Tor 4 years ago so Nyx is unmaintained. However, I love contributors! Hence why I still reply to pull requests like this. ;P

Reopening as you requested. Please talk with Tor's employees if you'd like to further pursue this.

A client program should verify the data it receives before processing it further.

Nyx's merely displays the data that it gets from Tor. There isn't any 'verification' that Nyx can do. Your patch merely replaces excessively high datapoints to work around Tor's bug.

I think that the proper route forward would be to ask David why he closed Tor's ticket for this.

@atagar atagar reopened this Jul 29, 2024
@excurso
Copy link
Author

excurso commented Jul 30, 2024

Hi atagar!

OK, I got it!

Your patch merely replaces excessively high datapoints to work around Tor's bug.

Yes

There isn't any 'verification' that Nyx can do.

There's already a verification in my fix. Namely whether a value is within the bandwidth burst limit. If it is not, you know it is invalid and can limit it to bandwidth burst at least. But to replace it with the average value is a more accurate solution, I think.

Sure, it would be better to have a fix for this in Tor.
I would look into this there, but have currently not the time for long debugging sessions. The "Mystery Bug" label implies such a long session... Later maybe... Because of this the quick fix in nyx.

Maybe it is the conversion between signed and unsigned int/long that causes the problem in Tor. When an signed int gets negative and then gets converted to an unsigned int, one might get such high unsigned values. But I have not verified it yet...

@omurad
Copy link

omurad commented Aug 4, 2024

I have traced the root cause of this issue.

  1. Tor stores the number of bytes read/written since the process started in stats_n_bytes_read/write
  2. Tor caches the last 300 bandwidth entries for bytes read/written over the past 1 second, so (theoretically) 5 minutes worth of data.
  3. When Tor updates its bandwidth cache it deducts the previous stats_n_bytes_read/write value from the current one, then the old cache entry is popped and the latest value is pushed to the front.
  4. Tor updates this cache every second **while a control connection is connected**.

This mechanism is what is causing graph spikes, when a Nyx disconnects then later reconnects to Tor, the latest bandwidth cache entry will be all the data read/written since last reading.

I see three options for addressing this:

  1. Tor: Update tor cache regardless of the existence of a control connection. (Difficult, and not ideal due to the overhead of constantly updating the cache, I assume it is for this reason it was implemented this way).
  2. Tor: Clear the bandwidth cache on control disconnect.
  3. Nyx: Ignore cached entries from before the app was launched. (Simple).

I plan on posting this response here as well (waiting on account approval), as this is not a "Mystery Bug". This way we can see what Tor thinks about this behavior, and get confirmation on whether it was intentionally designed this way.

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.

3 participants