Make sbws easy to understand and maintain

There was not an initial design and we are still adding functionality. Some of the new functionality require modifying several parts of the code and it's becoming harder to maintain and understand. This ticket is to identify the changes needed (refactors, etc.).

changed milestone to %sbws: unspecified

added component::core tor/sbws milestone::sbws: unspecified priority::medium severity::normal status::new type::defect labels

Pad with some ideas: https://pad.riseup.net/p/rGfvR7ZsvtoZ

These are some good ideas. But they might take a long time to do.

Do you want to do them after sbws 1.1?

If we just want to change variable names, we can do the changes in any version. If we want to change the keys in the data file, we should write code that reads the old keys, or do the changes in sbws 2.0. If we want to change the keys in the bandwidth file, we could do the changes in sbws 2.0 and bandwidth file format 2.0.

But no-one is parsing the bandwidth file yet, so if we want to make major changes, now would be a good time.

Replying to teor:

But no-one is parsing the bandwidth file yet, so if we want to make major changes, now would be a good time.

We can create a child ticket for changing the keys in the bandwidth file, do it after 1.0 and upgrade the other milestones to 2.X.

Replying to juga:

Replying to teor:

But no-one is parsing the bandwidth file yet, so if we want to make major changes, now would be a good time.

We can create a child ticket for changing the keys in the bandwidth file, do it after 1.0 and upgrade the other milestones to 2.X.

We could change the keys in the bandwidth file, or we could leave them as they are, and document the abbreviations. I am not sure if changing the keys is worth it. We would also have to change the spec. Edit: and fix any bugs that we create in the change

juga, do you think that changing the keys will make sbws a lot easier to maintain? If it won't, or if it will take a lot of time, we should just focus on fixing bugs and deploying sbws.

irl, how much work have you done on parsing the bandwidth file? Is now a good time to change some of the existing keys? Edit: complete question

Trac:
Cc: juga, teor to juga, teor, irl

I have not yet done any work on parsing the bandwidth file, and it's unlikely to happen this week.

For any files that are archived we should have parsers, so that means tjr's archive of torflow files and then whatever sbws files look like when we start archiving them.

If the file format is changing, and refactoring things is something you have time for, adding a bandwidth list formatter and parser to stem would allow it to be used in sbws and then also in bushel. There won't be any inter-op testing so we're more likely to miss bugs in spec compliance but there would be reduced code maintenance.

Replying to teor:

Replying to juga:

Replying to teor:

But no-one is parsing the bandwidth file yet, so if we want to make major changes, now would be a good time.

We can create a child ticket for changing the keys in the bandwidth file, do it after 1.0 and upgrade the other milestones to 2.X.

We could change the keys in the bandwidth file, or we could leave them as they are, and document the abbreviations. I am not sure if changing the keys is worth it. We would also have to change the spec. Edit: and fix any bugs that we create in the change

juga, do you think that changing the keys will make sbws a lot easier to maintain? If it won't, or if it will take a lot of time, we should just focus on fixing bugs and deploying sbws.

I agree that just changing the name of the keys doesn't affect much on the maintainability of sbws and i agree will take some time, however as you said, if we don't do it now we'll have those keys for longer time. I don't know...

Replying to irl:

If the file format is changing, and refactoring things is something you have time for, adding a bandwidth list formatter and parser to stem would allow it to be used in sbws and then also in bushel. There won't be any inter-op testing so we're more likely to miss bugs in spec compliance but there would be reduced code maintenance.

This sounds a good idea. I think that implementing the bandwidth file parsing in stem is just mostly copy-pasting what we implemented in sbws (without the scaling part and parsing results, it's not too complex nor too much code0. So, maybe instead of renaming the keys, as soon as there's time (in 1/2 weeks), i could try this. Later we can rename the keys there.

Replying to irl:

I have not yet done any work on parsing the bandwidth file, and it's unlikely to happen this week.

For any files that are archived we should have parsers, so that means tjr's archive of torflow files and then whatever sbws files look like when we start archiving them.

Ok, if we've already archived some torflow and sbws flies, then we shouldn't change the keys.

We can't just change the keys if we have old files that we'd want to parse. It would have to be a new version of the format and we'd need at least parsers (if not also formatters) for both.

When I say "archvied" I mean files that are eventually going to end up in CollecTor. Currently this is only tjr's files unless someone is archiving sbws bandwidth lists and can convince me that they are useful to archive (I am not aware of anyone doing this).

Replying to irl:

We can't just change the keys if we have old files that we'd want to parse. It would have to be a new version of the format and we'd need at least parsers (if not also formatters) for both.

When I say "archvied" I mean files that are eventually going to end up in CollecTor. Currently this is only tjr's files unless someone is archiving sbws bandwidth lists and can convince me that they are useful to archive (I am not aware of anyone doing this).

How is tjr archiving bandwidth files? From which authorities?

If tjr is archiving bandwidth files from longclaw, then those files are generated by sbws.

This one: https://bwauth.ritter.vg/bwauth/bwscan.V3BandwidthsFile

Maybe it is sbws.

It is not sbws. sbws has headers.

Now that sbws milestone 1.0 is mostly done, i could work on this before starting with 1.1 milestone. Should i then create a ticket to implement this in stem?.

irl, which should be the output of the parser?, json?.

If json, would it be something like?.

{
  "timestamp": int,
  # other keyvalues in header
  # bandwidth lines
  {
    # one bandwidth line
    {
      "bw": int,
      "nodeid": string,
      # other line's keyvalues
    }
  }
}

The output of the parser for stem should be a Python object. Take a look at the ServerDescriptor class for an example:

https://stem.torproject.org/api/descriptor/server_descriptor.html#stem.descriptor.server_descriptor.RelayDescriptor

Replying to irl:

The output of the parser for stem should be a Python object. Take a look at the ServerDescriptor class for an example:

https://stem.torproject.org/api/descriptor/server_descriptor.html#stem.descriptor.server_descriptor.RelayDescriptor

Oh, then it's even easier, since it's already an object (https://github.com/torproject/sbws/blob/master/sbws/lib/v3bwfile.py#L511). The things to do then:

1. Maybe solve first #28282 (moved) to eliminate the aggregation logic from the classes
2. To do not depend on a stem version that is not released, duplicate the code both in sbws and stem, until there's a new stem release

It would be a good idea to speak to atagar about what he would want to see for the parser/formatter. Ideally it would work in the same way as the existing ones which might need some functions renamed or with different arguments. Those bugs should be new tickets too, in the stem component.

i've created #29056 (closed) for this, we can now discuss this there.

mentioned in issue #28737 (moved)

mentioned in issue #29056 (closed)

mentioned in issue #29716 (moved)

mentioned in issue #29727 (moved)

mentioned in issue #29755 (moved)

mentioned in issue #29783 (moved)

mentioned in issue #29047 (moved)

mentioned in issue #29046 (moved)

mentioned in issue #29048 (moved)

mentioned in issue #29057 (moved)

mentioned in issue #29718 (moved)

mentioned in issue #29726 (moved)

mentioned in issue #29721 (moved)

mentioned in issue #29717 (moved)

mentioned in issue #33197 (moved)

mentioned in issue #33571 (moved)

moved to tpo/network-health/sbws#28684 (closed)

mentioned in issue tpo/network-health/sbws#29727 (closed)