596660 - reports.uptime appearing sometimes as null

Status: RESOLVED FIXED

(Socorro :: General, task, P2)

Description

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Some reports that are being fed into Postgres from hBase are being inserted with a default uptime value of null, instead of a default value of 0.  Uptime should be inserted with a default value of 0.

Comment 1

[Laura Thomson :laura]

This is the root cause of bug 596637

Comment 2

[Austin King [:ozten]]

@Assigning to rhelmer

This is the root cause of Bug#596637. We have bullet-proofed the cron so this isn't an issue for us.

We should go through and compare crash properties like uptime with the Postgres DB Schema and default values the same way that PG did.

Example:
reports table uptime default value 0

hbase/db feeder code should check for null or empty string and make this a 0 before inserting into the reports table.

Comment 3

[Robert Helmer [:rhelmer]]

I am pretty sure this is happening because StartupTime is not always sent by the client, and in this case an exception will be thrown here and uptime sent to None:

http://code.google.com/p/socorro/source/browse/trunk/socorro/processor/daemon_proc.py#886

The processor used in the previous release did this differently, using a method called "getJsonOrWarn()" to fall back to the value of timestampTime
 if StartupTime is unset:

http://code.google.com/p/socorro/source/browse/tags/releases/1.7.2_20100715/socorro/processor/processor.py#753

I hacked up daemon_proc.py a little to read a JSON file we got from the wild with no StartupTime, and the JSON that daemon_proc.py generates for HBase does have "'uptime':None" in it. From reading dbfeeder, I am pretty sure that this just inserts a NULL into PostgreSQL.

I am still getting my dev environment set up, so I'll have a go at a patch once I can test it end-to-end. I think making the processor a little more testable adding unit tests here would make these kinds of changes easier in the future too.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: set uptime to date_processed instead of null

This does the bare minimum to prevent another occurrence of bug 596637. The changes I propose in comment 3 are nice to have but should not be release-blocking. Maybe best to spin out separate bugs ("unit tests for processor", "uptime column should be NOT NULL in reports table").

Currently testing this, Lars let me know if you agree with the above and I'll ask for review once testing and any further changes are done.

Comment 5

[Robert Helmer [:rhelmer]]

Attachment: set uptime to 0 instead of None

This is the same as attachment 479837 [details] [diff] [review] except it sets uptime to 0 instead of date_processed (which was not correct anyway, I had meant to do max(0,processed_time - startupTime) which is what the old code ended up doing in a roundabout way, but hadn't done it yet) and the log message I added has been removed.

The more I look at it I don't think it would ever not be 0 in this case if crash_time is not supplied by the client, due to the max(0,...):
http://code.google.com/p/socorro/source/browse/tags/releases/1.7.2_20100715/socorro/processor/processor.py#765

In any case, I think this makes more sense because we don't know why the exception was thrown, it could have been a problem in client_crash_time or client_startup_time so we should cover our bases.

Comment 6

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 479847 [details] [diff] [review]
set uptime to 0 instead of None

looks fine to me.

Comment 7

[Robert Helmer [:rhelmer]]

Finally got around to testing this locally now that I have everything set up; inserting a crash with missing StartupTime causes the update column to be set to 0 rather than null with this patch.

Committed revision 2627.