Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21587 closed defect (fixed)

Improve running bridges statistic by skipping statuses without any running bridges

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

Description

When we calculate running bridges we compute the mean value of running bridges of all bridge network statuses published on a given day. This includes bridge network statuses without any running bridges, that is, with zero included bridges having the Running flag assigned. I assume that the main reason for this case is that the bridge authority got restarted. This only happens once every few weeks, but it happens, and it impacts the mean value.

What we could do to fix the statistics here is require at least one bridge with the Running flag in a bridge network status, or we don't consider that status.

Here's a quick analysis of the problem and the suggested fix based on internal data written by metrics-web that I'll also attach to this ticket:

b <- read.csv("bridge-consensus-stats-raw", stringsAsFactors = FALSE)
b <- b[b$authority == "Bifroest", ]
b <- b[as.Date(b$datetime) %in% as.Date(b[b$brunning == 0, "datetime"]), ]
print(b)
print(aggregate(b$brunning, by = list(date = as.Date(b$datetime)), FUN = mean))
b <- b[b$brunning > 0, ]
print(aggregate(b$brunning, by = list(date = as.Date(b$datetime)), FUN = mean))
                  datetime authority brunning brunningec2
108757 2016-08-31 00:42:32  Bifroest     1572           0
108759 2016-08-31 01:42:32  Bifroest     1567           0
108761 2016-08-31 02:42:32  Bifroest     1567           0
108763 2016-08-31 03:42:32  Bifroest     1567           0
108765 2016-08-31 04:42:32  Bifroest     1566           0
108767 2016-08-31 05:42:32  Bifroest     1568           0
108769 2016-08-31 06:42:32  Bifroest     1572           0
108771 2016-08-31 07:42:32  Bifroest     1570           0
108773 2016-08-31 08:42:32  Bifroest     1571           0
108775 2016-08-31 09:42:32  Bifroest     1573           0
108777 2016-08-31 10:38:15  Bifroest        0           0
108779 2016-08-31 11:38:15  Bifroest     1574           0
108781 2016-08-31 12:38:16  Bifroest     1575           0
108783 2016-08-31 13:38:16  Bifroest     1572           0
108785 2016-08-31 14:38:16  Bifroest     1576           0
108787 2016-08-31 15:38:16  Bifroest     1577           0
108789 2016-08-31 16:38:16  Bifroest     1580           0
108791 2016-08-31 17:38:16  Bifroest     1582           0
108793 2016-08-31 18:38:16  Bifroest     1581           0
108795 2016-08-31 19:38:16  Bifroest     1580           0
108797 2016-08-31 20:38:16  Bifroest     1581           0
108799 2016-08-31 21:38:16  Bifroest     1587           0
108801 2016-08-31 22:38:16  Bifroest     1584           0
108803 2016-08-31 23:38:16  Bifroest     1581           0
109969 2016-10-18 00:38:17  Bifroest     2026           0
109970 2016-10-18 01:38:17  Bifroest     2026           0
109971 2016-10-18 02:38:17  Bifroest     2023           0
109972 2016-10-18 03:38:17  Bifroest     2030           0
109973 2016-10-18 04:38:17  Bifroest     2036           0
109974 2016-10-18 05:38:17  Bifroest     2036           0
109975 2016-10-18 06:38:17  Bifroest     2032           0
109976 2016-10-18 07:38:17  Bifroest     2037           0
109977 2016-10-18 08:38:17  Bifroest     2036           0
109978 2016-10-18 09:38:17  Bifroest     2041           0
109979 2016-10-18 10:38:17  Bifroest     2038           0
109980 2016-10-18 11:38:17  Bifroest     2036           0
109981 2016-10-18 12:38:17  Bifroest     2033           0
109982 2016-10-18 13:38:17  Bifroest     2031           0
109983 2016-10-18 14:38:17  Bifroest     2032           0
109984 2016-10-18 15:41:01  Bifroest        0           0
109985 2016-10-18 16:41:01  Bifroest     2036           0
109986 2016-10-18 17:41:01  Bifroest     2021           0
109987 2016-10-18 18:41:01  Bifroest     2039           0
109988 2016-10-18 19:41:01  Bifroest     2042           0
109989 2016-10-18 20:41:01  Bifroest     2035           0
109990 2016-10-18 21:41:01  Bifroest     2033           0
109991 2016-10-18 22:41:01  Bifroest     2032           0
109992 2016-10-18 23:41:01  Bifroest     2030           0
112295 2017-01-23 00:41:04  Bifroest     2649           0
112296 2017-01-23 01:41:04  Bifroest     2647           0
112297 2017-01-23 02:41:04  Bifroest     2642           0
112298 2017-01-23 03:41:04  Bifroest     2643           0
112299 2017-01-23 04:41:04  Bifroest     2643           0
112300 2017-01-23 05:41:04  Bifroest     2641           0
112301 2017-01-23 06:41:04  Bifroest     2643           0
112302 2017-01-23 07:41:04  Bifroest     2644           0
112303 2017-01-23 08:41:04  Bifroest     2645           0
112304 2017-01-23 09:41:04  Bifroest     2648           0
112305 2017-01-23 10:41:04  Bifroest     2652           0
112306 2017-01-23 11:41:04  Bifroest     2655           0
112307 2017-01-23 12:41:04  Bifroest     2654           0
112308 2017-01-23 13:41:04  Bifroest     2655           0
112309 2017-01-23 14:41:04  Bifroest     2655           0
112310 2017-01-23 15:41:04  Bifroest     2652           0
112311 2017-01-23 16:41:04  Bifroest     2650           0
112312 2017-01-23 17:41:04  Bifroest     2653           0
112313 2017-01-23 18:41:04  Bifroest     2658           0
112314 2017-01-23 19:41:04  Bifroest     2648           0
112315 2017-01-23 20:41:04  Bifroest     2661           0
112316 2017-01-23 21:41:04  Bifroest     2656           0
112317 2017-01-23 22:41:04  Bifroest     2658           0
112318 2017-01-23 23:57:27  Bifroest        0           0
        date        x
1 2016-08-31 1509.292
2 2016-10-18 1948.375
3 2017-01-23 2539.667
        date        x
1 2016-08-31 1574.913
2 2016-10-18 2033.087
3 2017-01-23 2650.087

Note that the "restart" theory is supported by the change in publication times starting with the status containing zero running bridges.

Also note change in statistics when omitting statuses with zero bridges.

Child Tickets

Attachments (1)

bridge-consensus-stats-raw.xz (286.0 KB) - added by karsten 2 years ago.

Download all attachments as: .zip

Change History (6)

Changed 2 years ago by karsten

comment:1 Changed 2 years ago by karsten

Status: newneeds_review

Here's a possible fix:

diff --git a/modules/legacy/src/main/java/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java b/modules/legacy/src/main/java/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
index 9aef3e4..801794f 100644
--- a/modules/legacy/src/main/java/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
+++ b/modules/legacy/src/main/java/org/torproject/ernie/cron/network/ConsensusStatsFileHandler.java
@@ -257,17 +257,23 @@ public class ConsensusStatsFileHandler {
      * and bridge authority. */
     Map<String, Map<String, int[]>> bridgesPerDayAndAuthority = new HashMap<>();
     for (String bridgesRawLine : this.bridgesRaw.values()) {
+      String[] parts = bridgesRawLine.split(",");
+      int brunning = Integer.parseInt(parts[2]);
+      if (brunning <= 0) {
+        /* Skip this status which contains zero bridges with the Running
+         * flag. */
+        continue;
+      }
       String date = bridgesRawLine.substring(0, 10);
       if (!bridgesPerDayAndAuthority.containsKey(date)) {
         bridgesPerDayAndAuthority.put(date, new TreeMap<String, int[]>());
       }
-      String[] parts = bridgesRawLine.split(",");
       String authority = parts[1];
       if (!bridgesPerDayAndAuthority.get(date).containsKey(authority)) {
         bridgesPerDayAndAuthority.get(date).put(authority, new int[3]);
       }
       int[] bridges = bridgesPerDayAndAuthority.get(date).get(authority);
-      bridges[0] += Integer.parseInt(parts[2]);
+      bridges[0] += brunning;
       bridges[1] += Integer.parseInt(parts[3]);
       bridges[2]++;
     }

comment:2 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

The reasoning makes sense and the fix looks ok.
Maybe, log a warning for the zero running bridges case?

comment:3 Changed 2 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

I looked into logging a warning in this case, but we're re-processing all known bridge statuses twice per day, and we'd see that warning dozens or hundreds of time in each execution. I think even on level finer it would be too noisy. That's why I left out the log message.

Cherry-picked and pushed to master. Thanks for looking! Closing.

comment:4 in reply to:  3 ; Changed 2 years ago by iwakeh

Replying to karsten:

I looked into logging a warning in this case, but we're re-processing all known bridge statuses twice per day, and we'd see that warning dozens or hundreds of time in each execution. I think even on level finer it would be too noisy. That's why I left out the log message.

That's a very valid concern.
There would still be debug level, but on the other hand zero running bridges due to other reasons should be noticeable in a different way.
So all should be ok here.

comment:5 in reply to:  4 Changed 2 years ago by karsten

Replying to iwakeh:

Replying to karsten:

I looked into logging a warning in this case, but we're re-processing all known bridge statuses twice per day, and we'd see that warning dozens or hundreds of time in each execution. I think even on level finer it would be too noisy. That's why I left out the log message.

That's a very valid concern.
There would still be debug level, [...]

Heh, no, we're still using java.util.logging.* in this code, so finer is probably comparable to debug.

[...] but on the other hand zero running bridges due to other reasons should be noticeable in a different way.
So all should be ok here.

Alright, leaving this closed.

Note: See TracTickets for help on using tickets.