Opened 4 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#31780 closed task (implemented)

Write a specification for BridgeDB's metrics

Reported by: phw Owned by: phw
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Normal Keywords:
Cc: phw, karsten Actual Points: 0.7
Parent ID: Points: 0.5
Reviewer: cohosh Sponsor: Sponsor30-can

Description

Now that BridgeDB exports metrics (#9316), it's time to specify the format of these metrics, allowing third parties to write parsers.

Child Tickets

Change History (15)

comment:1 Changed 4 weeks ago by phw

Cc: karsten added
Reviewer: cohosh
Status: assignedneeds_review

I wrote a patch that adds BridgeDB's metrics format to the torspec repository.

Note that the patch differs from our current implementation in its version number. Our current implementation uses 1.0 while the spec uses 1. I would like to update the implementation because a single number number seems simpler.

comment:2 Changed 4 weeks ago by cohosh

Status: needs_reviewneeds_revision

I left a few comments on the commit.

Also note that the snowflake metrics spec exists in the snowflake repository and not in torspec (as suggested in https://trac.torproject.org/projects/tor/ticket/28942#comment:65)
I know there's already a spec for bridgedb in torspec so appending this to that makes sense to me. Just noting the difference here.

comment:3 in reply to:  2 Changed 4 weeks ago by phw

Status: needs_revisionneeds_review

Replying to cohosh:

I left a few comments on the commit.


Thanks! I addressed your feedback here.

Also note that the snowflake metrics spec exists in the snowflake repository and not in torspec (as suggested in https://trac.torproject.org/projects/tor/ticket/28942#comment:65)
I know there's already a spec for bridgedb in torspec so appending this to that makes sense to me. Just noting the difference here.


Good point. BridgeDB's specification is outdated to a point where it may do more harm than good and if somebody's interested in the metrics spec, they are more likely to search for it in BridgeDB's repo. I think we should move it. Here's a patch that adds the spec to BridgeDB's repo and updates the code to be consistent with the spec. (The spec includes the revisions, so you don't have to re-review it.)

Last edited 4 weeks ago by phw (previous) (diff)

comment:4 Changed 4 weeks ago by cohosh

Status: needs_reviewmerge_ready

These revisions look good to me!

comment:5 in reply to:  4 Changed 4 weeks ago by phw

Replying to cohosh:

These revisions look good to me!


Thanks. I'll wait a bit before merging because karsten mentioned he'll take a look too.

comment:6 Changed 2 weeks ago by karsten

Sorry for the delay. I'll take a look today!

comment:7 Changed 2 weeks ago by karsten

Just one question: can we somehow make these changes to existing bridgedb-metrics.log* files, either on polyanthum or on colchicifolium? It would be sad to lose a month of files just because of cosmetic changes to the spec. If so, do you prefer editing files yourself, or shall I do that?

comment:8 in reply to:  7 ; Changed 2 weeks ago by phw

Replying to karsten:

Just one question: can we somehow make these changes to existing bridgedb-metrics.log* files, either on polyanthum or on colchicifolium? It would be sad to lose a month of files just because of cosmetic changes to the spec. If so, do you prefer editing files yourself, or shall I do that?


I will modify all the files that are currently on polyanthum but you'll have to modify the ones that were already synced to colchicifolium.

Other than that, does the spec look good to you?

comment:9 in reply to:  8 ; Changed 2 weeks ago by phw

Replying to phw:

Replying to karsten:

Just one question: can we somehow make these changes to existing bridgedb-metrics.log* files, either on polyanthum or on colchicifolium? It would be sad to lose a month of files just because of cosmetic changes to the spec. If so, do you prefer editing files yourself, or shall I do that?


I will modify all the files that are currently on polyanthum but you'll have to modify the ones that were already synced to colchicifolium.


Ok, this is done. The updated files should soon be synced to colchicifolium.

Here's an implementation quirk that I just realised: When I restart BridgeDB (e.g., to update to the latest version), it does not write its unfinished metrics to disk, which means that we are losing up to 24 hours worth of metrics after each restart. I filed #31936 for this issue.

comment:10 in reply to:  8 ; Changed 2 weeks ago by karsten

Replying to phw:

Replying to karsten:

Just one question: can we somehow make these changes to existing bridgedb-metrics.log* files, either on polyanthum or on colchicifolium? It would be sad to lose a month of files just because of cosmetic changes to the spec. If so, do you prefer editing files yourself, or shall I do that?


I will modify all the files that are currently on polyanthum but you'll have to modify the ones that were already synced to colchicifolium.

Looks like all files on colchicifolium are already updated. Nothing left to do for me, unless I'm overlooking something.

Other than that, does the spec look good to you?

Sure, it looks fine to me. My main concern was that we'd have to update existing files and code, but it's much easier to do this now than later, so I'm okay with that.

Can I update the code to this new spec now, or do you expect to make any further changes to version 1?

comment:11 in reply to:  9 ; Changed 2 weeks ago by karsten

Replying to phw:

Here's an implementation quirk that I just realised: When I restart BridgeDB (e.g., to update to the latest version), it does not write its unfinished metrics to disk, which means that we are losing up to 24 hours worth of metrics after each restart. I filed #31936 for this issue.

Relays have the same issue. The downside of writing unfinished metrics to disk is that having non-sanitized metrics on disk can be a security problem. This is why relays keep non-sanitized statistics in memory, then sanitize them, and then write them to disk. Of course it's unfortunate to lose up to 24 hours worth of metrics because of a restart, but for relays this hasn't caused major trouble in the past. Maybe this is different with BridgeDB, though.

Anyway, this is unrelated to making BridgeDB stats/metrics available, right? If so, I'd continue with adding these stats/metrics to metrics-lib and CollecTor.

comment:12 in reply to:  10 Changed 2 weeks ago by phw

Replying to karsten:

Can I update the code to this new spec now, or do you expect to make any further changes to version 1?


There will be no further changes to version 1, so please update the code.

comment:13 in reply to:  11 Changed 2 weeks ago by phw

Replying to karsten:

Replying to phw:

Here's an implementation quirk that I just realised: When I restart BridgeDB (e.g., to update to the latest version), it does not write its unfinished metrics to disk, which means that we are losing up to 24 hours worth of metrics after each restart. I filed #31936 for this issue.

Relays have the same issue. The downside of writing unfinished metrics to disk is that having non-sanitized metrics on disk can be a security problem. This is why relays keep non-sanitized statistics in memory, then sanitize them, and then write them to disk. Of course it's unfortunate to lose up to 24 hours worth of metrics because of a restart, but for relays this hasn't caused major trouble in the past. Maybe this is different with BridgeDB, though.


Thanks for the context, that's helpful to know.

Anyway, this is unrelated to making BridgeDB stats/metrics available, right? If so, I'd continue with adding these stats/metrics to metrics-lib and CollecTor.


Yes, it is.

comment:14 Changed 2 weeks ago by phw

Resolution: implemented
Status: merge_readyclosed

I merged the patch in 0751ad7.

comment:15 Changed 2 weeks ago by phw

Actual Points: 0.7
Note: See TracTickets for help on using tickets.