Opened 10 months ago

Closed 3 months ago

#25177 closed enhancement (fixed)

Remove redundant clients graphs

Reported by: karsten Owned by: karsten
Priority: Medium Milestone: Onionoo 1.18.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As part of #24729 we found that the "1_week" and "1_month" graphs in clients documents are redundant, because they have the same data resolution of 1 day as the "3_months" graph. We should consider dropping the "1_week" and "1_month" graphs and teaching clients to simply use data from the "3_months" graph instead.

Child Tickets

Change History (10)

comment:1 Changed 5 months ago by karsten

Cc: metrics-team added
Priority: LowMedium
Summary: Remove redundant clients graphsRemove clients documents

I'm making this ticket more general by suggesting to remove all clients documents, not just the ones containing redundant information. The reason is that even though we added clients documents 4+ years ago (#10331), I'm not aware of any Onionoo client using or planning to use this data. This seems like an easy way to reduce code complexity a bit, by removing an unused feature.

Requires a major protocol update with 1 month prior notice. If we conclude on this ticket that clients documents can go away, we can start that 1 month period, and if somebody shows up with a plausible use case why they really need our clients documents, we can leave them in.

Thoughts?

comment:2 Changed 5 months ago by irl

Relay Search would be one client I can think of that uses them...

https://metrics.torproject.org/rs.html#details/36023F6479610E64A1F3015B5DC85A181E6995AB

comment:3 Changed 5 months ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted
Summary: Remove clients documentsRemove redundant clients graphs

Oh! I somehow misremembered that by thinking of the "countries", "transports", and "versions" fields in clients documents that Atlas/Relay Search never used and that we took out last year. Back to removing redundant clients graphs! I'll start looking into that. Heh.

comment:4 Changed 5 months ago by karsten

Milestone: Onionoo 1.16.0

Planning to do this for the upcoming release.

comment:5 Changed 5 months ago by karsten

Status: acceptedneeds_review

Please review commit 7f067d7 in my task-25177 branch.

Not sure if we already discussed this, but I think we should make this change together with the next major protocol bump in about a month from now. The average_clients field may be optional, but we state in the specification that "Keys are fixed strings "1_week", "1_month", "3_months", [...]". Maybe we should give our users this month as advance notice.

comment:6 Changed 5 months ago by irl

Status: needs_reviewmerge_ready

LGTM.

Relay Search, at least, is not expecting fixed strings and does parse the keys. No objection to including this in the major protocol bump to give users more notice of the change.

merge_ready for Onionoo change, but do you also have a spec diff? (I guess it is trivial)

comment:7 in reply to:  6 Changed 5 months ago by karsten

Replying to irl:

LGTM.

Great!

Relay Search, at least, is not expecting fixed strings and does parse the keys. No objection to including this in the major protocol bump to give users more notice of the change.

Alright!

merge_ready for Onionoo change, but do you also have a spec diff? (I guess it is trivial)

I don't have a spec diff yet, but I'll be sure to put out one for review that covers all changes for the next minor and the next major version. And yes, the diff for this change in particular is likely going to be trivial.

Leaving this in merge_ready for merging it as soon as we're putting out the next major version. Thanks!

comment:8 Changed 4 months ago by karsten

Milestone: Onionoo 1.16.0Onionoo 1.17.0

Schedule for protocol version 7.0, likely to be released in 1.17.0.

comment:9 Changed 3 months ago by karsten

Milestone: Onionoo 1.17.0Onionoo 1.18.0

Versions 1.17.0 and 1.17.1 are already released, the next released version will be 1.18.0.

comment:10 Changed 3 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

This is now merged and deployed as part of 7.0-1.18.1. Closing. Thanks!

Note: See TracTickets for help on using tickets.