Opened 13 months ago

Last modified 9 days ago

#28280 needs_revision enhancement

control: Add a key to GETINFO to get the DoS subsystem stats

Reported by: dgoulet Owned by:
Priority: Low Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-control, tor-spec, needs-spec
Cc: Actual Points: 0.1
Parent ID: Points:
Reviewer: teor, dgoulet Sponsor:

Description

The DoS subsystem keeps track of statistics that are logged in the heartbeat with dos_log_heartbeat(). I would like those to be exposed to the control port so it can be collected and graph over time (improve relay monitoring).

(Follows the idea behind #28279)

Child Tickets

Change History (6)

comment:1 Changed 11 days ago by moonsikpark

PR: https://github.com/torproject/tor/pull/1568

I'd like to make changes to control-spec.txt after, not before this PR's review, if that's OK.

Last edited 11 days ago by moonsikpark (previous) (diff)

comment:2 Changed 11 days ago by dgoulet

Status: newneeds_review

comment:3 Changed 11 days ago by teor

Actual Points: 0.1
Keywords: needs-spec added
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Reviewer: teor, dgoulet

Thanks for this code!

I'd like to make changes to control-spec.txt after, not before this PR's review, if that's OK.

I think it would be best if we agreed on a design first? Having a spec is a good way to agree on a design.

A few design notes:

  1. We might want a dos GETINFO category, with subcategories for each type of DoS.
  2. I don't know if we will want individual keys in each subcategory.
  3. We can do rollups of each sub-category and category, if we want.
  4. We should list all DoS variables as keys, including the *_enabled variables

A few formatting notes:

  1. All the names should be in a consistent format.
  2. We probably want to use the existing lowercase-hyphen format.
  3. Most of our controller names aren't plural.
  4. All values should be numeric (not False)
  5. We probably want values separated by newlines, to match existing GETINFOs. See downloads/ for an example. https://github.com/torproject/torspec/blob/master/control-spec.txt#L1008

A few implementation notes:

  1. The code will be easier to maintain if you use smartlist_add_asprintf() for each key=value, and smartlist_join_strings() at the end.
  2. The code will be even easier to maintain if you write a function that checks *_enabled, formats the key=value, and returns a newly allocated string. Then you can just pass the key name, the *_enabled variable, and the actual variable to the function.

There are also some minor issues with the code, I added comments on the pull request.

I'd like to check this advice with the developer who wrote the DoS code, so I'm assigning this ticket to them for review.

comment:4 in reply to:  3 ; Changed 11 days ago by moonsikpark

I agree that changing control-spec first is the way to go.

I'll wait for dgoulet's comment on how this should be done.

comment:5 in reply to:  4 Changed 9 days ago by dgoulet

Replying to moonsikpark:

I agree that changing control-spec first is the way to go.

I'll wait for dgoulet's comment on how this should be done.

Yes, what teor proposed is really the way to go.

@moonsikpark, you can simply make a branch to the tor-spec.git and add the dos category to GETINFO like teor suggested. Do a first pass and we'll go from there :).

comment:6 Changed 9 days ago by teor

Status: needs_reviewneeds_revision

Please set this ticket to needs_review when you have done the control spec pull request.

We accept spec pull requests on GitHub here:
https://github.com/torproject/torspec

Note: See TracTickets for help on using tickets.