Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#27039 closed defect (fixed)

Timestamps in graph history documents are incorrectly formatted

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

Description (last modified by irl)

Timestamps in graph history documents are being served as ints (maybe unix timestamps) instead of YYYY-MM-DD hh:mm:ss format.

This has the result of breaking Relay Search's graphs and also of breaking the t-shirt script.

Child Tickets

Change History (22)

comment:1 Changed 4 months ago by nusenu

#27041 has been closed as a duplicate of this

comment:2 Changed 4 months ago by irl

The t-shirt script is also broken, likely for the same reason.

https://gitweb.torproject.org/metrics-tasks.git/tree/task-9889/tshirt.py

comment:3 Changed 4 months ago by irl

Owner: changed from metrics-team to irl
Status: newaccepted

Will try to look at this tomorrow with other relay search tickets.

comment:4 Changed 4 months ago by kat5

Here's the script output:

kat@arbour:~/tor/relayops/tshirts$ ./tshirt.py 097CD81AB87D2F5DDB2538CF1221D978044F3323
Fetched bandwidth document
Fetched uptime document
Fetched details document
Traceback (most recent call last):

File "./tshirt.py", line 197, in <module>

check_tshirt(search_query)

File "./tshirt.py", line 183, in check_tshirt

uptime_percent = get_uptime_percent(uptime_data[i])

File "./tshirt.py", line 149, in get_uptime_percent

return round(calculate_2mo_avg(response, 'uptime') * 100, 2)

File "./tshirt.py", line 102, in calculate_2mo_avg

first = datetime.strptime(datafirst?, "%Y-%m-%d %H:%M:%S")

TypeError: strptime() argument 1 must be string, not int
kat@arbour:~/tor/relayops/tshirts$

comment:5 Changed 4 months ago by irl

Cc: metrics-team added

comment:6 Changed 4 months ago by irl

This is definitely the same problem. The Relay Search error is that it can't perform a string split on an int.

comment:7 Changed 4 months ago by irl

Component: Metrics/Relay SearchMetrics/Onionoo
Description: modified (diff)
Priority: MediumHigh
Summary: Relay Search graphs missingTimestamps in graph history documents are incorrectly formatted

I think it's possible that the getter is being used instead of the variable, which parses the String to return a long.

https://gitweb.torproject.org/onionoo.git/tree/src/main/java/org/torproject/onionoo/docs/GraphHistory.java#n16

comment:8 Changed 4 months ago by karsten

Yes, I think that's the issue. Looks like we did not include the GraphHistory class when we added Jackson-related annotations in 52494a7.

The fix is probably as simple as copying those annotations from Document to GraphHistory.

But how do we fix existing files in out/ directories?

  • We could wait until new files are being written and accept that old files might be broken. Not very nice.
  • We could change the servlet to read all history files and rewrite them before including them in a response. Not very efficient.
  • We could write a small tool to run on an existing out/ directory that reads in history files and overwrites them, if they are broken. Might be okay.

comment:9 Changed 4 months ago by irl

karsten - I've been looking at other Relay Search tickets and was going to loop back round to this one. The option with the small tool looks like the best path to take, do you want to take this instead as you'll probably be able to do it quicker than me?

comment:10 Changed 4 months ago by karsten

I'll start with the small tool and will let you know here how that works. Feel free to start with the actual patch in the meantime, but if you're busy with other tickets, I'll do that after finishing the tool.

comment:11 Changed 3 months ago by karsten

Status: acceptedneeds_review

Please review commit 18e5c04 in my task-27039 branch, which is the small tool that rewrites broken history documents.

I'll now work on the actual fix.

comment:12 Changed 3 months ago by karsten

And please review commit 3be9f20 in my task-27039-2 branch with the actual fix.

comment:13 Changed 3 months ago by irl

Reviewer: irl

Will look at this today hopefully.

comment:14 Changed 3 months ago by irl

Ok. I want to run this on the real Onionoo data we have so that we're only fixing it once, so will need to wait for this to sync locally. I'm also going to try and do something clever with ZFS snapshots to make this kind of testing faster for me to get set up in the future (e.g. for #27050).

comment:15 Changed 3 months ago by irl

Owner: changed from irl to karsten
Status: needs_reviewassigned

comment:16 Changed 3 months ago by irl

Status: assignedneeds_review

comment:17 Changed 3 months ago by irl

Status: needs_reviewmerge_ready

Ok. I've run this on a local copy of the Onionoo data directory. For the deployment, I'll document the steps I took during testing, so we can just duplicate these. These instructions will also be applicable to anyone else upgrading from 6.2-1.16.0.

  • Checkout commit 18e5c04 in karsten's task-27039 branch
  • Run ant jar to build the jar file at generated/dist/onionoo-6.2-1.16.0-dev.jar. (You will not need the war file built.)
  • Upload this jar file along with the jar and war files for the 6.2-1.16.1 release.
  • Wait for hourly updater to end.
  • Stop hourly updater and the web server.
  • Run java -jar onionoo-6.2-1.16.0-dev.jar --rewrite-only.
  • Remove the intermediate jar file to avoid confusion.
  • Re-start the web server and the hourly updater using the files from the 6.2-1.16.1 release.
Last edited 3 months ago by irl (previous) (diff)

comment:18 Changed 3 months ago by karsten

Sounds like a good plan! Please find #27113 for the release part.

comment:19 Changed 3 months ago by Vort

@karsten, one more 500 error popped out: link. Is it related to this report or separate report is needed?

comment:20 Changed 3 months ago by irl

The above in comment:18 is unrelated.

comment:21 Changed 3 months ago by irl

Resolution: fixed
Status: merge_readyclosed

This is merged and deployed.

comment:22 in reply to:  19 Changed 3 months ago by karsten

Replying to Vort:

@karsten, one more 500 error popped out: link. Is it related to this report or separate report is needed?

Oops, fixed. It was indeed unrelated.

Note: See TracTickets for help on using tickets.