Opened 9 months ago

Closed 9 months ago

#28136 closed defect (fixed)

OnionPerf graph handles missing data on 2018-10-01 in a weird way

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

Description

https://metrics.torproject.org/torperf.html?start=2018-07-24&end=2018-10-22&source=all&server=onion&filesize=50kb

Unclear if it's a visualization or statistics bug. Going for Metrics/Website for now.

Child Tickets

Change History (9)

comment:1 Changed 9 months ago by karsten

Priority: MediumHigh
Status: assignedneeds_review

comment:2 Changed 9 months ago by irl

Status: needs_reviewmerge_ready

Looks good to me. I hope the visualisations don't look too depressing once the numbers are real.

comment:3 Changed 9 months ago by karsten

Thanks for looking! Merged and deployed, though it'll take until this evening for the change to be visible. Leaving this ticket open until we're sure the issue is resolved.

comment:4 Changed 9 months ago by karsten

Status: merge_readyneeds_review

Looks like it didn't work. And I found out why. Please also review commit e7c0c84 in my task-28136-2 branch for the second half of the fix.

comment:5 Changed 9 months ago by irl

Status: needs_reviewmerge_ready

Ok. This change looks good.

There is another function to sanitize the data from the database, emptyNull, which perhaps could do with either a better name or some JavaDoc. I can see what it does looking at the code but was confused looking at the code above that called it.

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

Replying to irl:

Ok. This change looks good.

Great! Merged and deployed. The update is going to take until tomorrow afternoon.

There is another function to sanitize the data from the database, emptyNull, which perhaps could do with either a better name or some JavaDoc. I can see what it does looking at the code but was confused looking at the code above that called it.

Agreed, there's room for improvement here. I'll think more about it and post a patch here tomorrow.

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

Status: merge_readyneeds_review

Replying to karsten:

Replying to irl:

There is another function to sanitize the data from the database, emptyNull, which perhaps could do with either a better name or some JavaDoc. I can see what it does looking at the code but was confused looking at the code above that called it.

Agreed, there's room for improvement here. I'll think more about it and post a patch here tomorrow.

Please review commit cdab81b in my task-28136-3 branch.

comment:8 Changed 9 months ago by irl

Status: needs_reviewmerge_ready

I think there's still some confusion between the functions. One is ensuring that nulls are present, the other is ensuring that nulls are not present. This is addressed by the comments but I wonder if they could have better names in the future. We shouldn't hold up deploying this though, this is looking good.

comment:9 in reply to:  8 Changed 9 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Replying to irl:

I think there's still some confusion between the functions. One is ensuring that nulls are present, the other is ensuring that nulls are not present. This is addressed by the comments but I wonder if they could have better names in the future. We shouldn't hold up deploying this though, this is looking good.

Okay. Merged and deployed. Closing. Thanks!

Note: See TracTickets for help on using tickets.