718901 - report/list missing reports when date argument is present (e.g. in links from TCBS)

Status: RESOLVED FIXED

(Socorro :: Webapp, task)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://crash-stats.mozilla.com/report/list?range_value=1&range_unit=days&date=2012-01-16&signature=_ZN8SkBitmapaSERKS_&version=FennecAndroid%3A12.0a1

link taken from : https://crash-stats.mozilla.com/topcrasher/byversion/FennecAndroid/12.0a1/1/all

It does appear if you do an advanced search :
https://crash-stats.mozilla.com/report/list?product=FennecAndroid&query_search=signature&query_type=contains&query=_ZN8SkBitmapaSERKS&reason_type=contains&date=01%2F17%2F2012%2017%3A38%3A13&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=_ZN8SkBitmapaSERKS_

Comment 1

[Robert Kaiser]

Hah, when removing the "date" field it works!

Also seen with https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2012-01-17&signature=js%3A%3Amjit%3A%3AImmutableSync%3A%3AallocReg&version=FennecAndroid%3A12.0a1 (linked from 12.0a1 top crasher report, #1 spot) vs. https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&signature=js%3A%3Amjit%3A%3AImmutableSync%3A%3AallocReg&version=FennecAndroid%3A12.0a1

Comment 2

[Robert Helmer [:rhelmer]]

Hmm ok the problem here seems to be that the end_date being returned by the topCrashBySignatureTrends service call is returning one day too early.

I can repro this on dev, and for an URL of:

http://socorro-api-dev-internal/bpapi/topcrash/sig/trend/rank/p/FennecAndroid/v/12.0a1/type/browser/end/2012-01-18T13%3A00%3A00%2B0000/duration/168/listsize/300

In the JSON response I see:
"end_date": "2012-01-17"

I'll look at the way end_date is constructed in the code, could very well be a UTC bug.

This result is then used by the webapp to build the /report/list URL such as:
https://crash-stats-dev.allizom.org/report/list?range_value=7&range_unit=days&date=2012-01-17&signature=mozalloc_abort%20%7C%20__swrite%20%7C%20dexDataMapAlloc&version=FennecAndroid%3A12.0a1

Comment 3

[Robert Helmer [:rhelmer]]

Ok I traced this out, here is where the /end/ param passed to the service '2012-01-18 13:00:00+00:00' becomes '2012-01-17':

breakpad=# SELECT
breakpad-# max(report_date)
breakpad-# FROM
breakpad-#   tcbs JOIN product_versions USING (product_version_id)
breakpad-# WHERE
breakpad-# tcbs.report_date <= '2012-01-18 13:00:00+00:00'
breakpad-# AND product_name = 'FennecAndroid'
breakpad-# AND version_string = '12.0a1'
breakpad-# ;
    max     
------------
 2012-01-17
(1 row)

So this restricts the /report/list results to only be as accurate as the tcbs report contents (which is updated nightly).

I think the bug is coming from this change in the webapp:
https://github.com/mozilla/socorro/commit/5884d4e0213bd800b8c7fab3be879fabcbe307d0

The old behavior was to filter dates between $startDate and (end of $endDate), e.g.:

BETWEEN utc_day_begins_pacific(($date::DATE - $interval::INTERVAL)::DATE) AND utc_day_ends_pacific($date)

The new code does:

BETWEEN date_trunc('day', (TIMESTAMPTZ $date - INTERVAL $interval )) AND $date

I think that's going to use the beginning of $date as the cutoff, instead of the end of $date.

Comment 4

[Robert Helmer [:rhelmer]]

OK actually looking at this closer, I think that there were two bugs before:

1) tcbs service considers 2012-01-17 to be the end of the period, so is considering the range:

2012-01-10 through 2012-01-17 - this is not correct, '2012-01-17 23:59:59' is the end of the period, so really it should be:

'2012-01-10 23:59:59' through '2012-01-17 23:59:59'

Or since we know that tcbs only runs once per day, we could simply do:

'2012-01-11' through '2012-01-18'

2) /report/list used to explicitly consider the "end date" passed to it to be the "end of the day", but that's actually a fairly recent fix from bug 679065

In summary, I think I fixed the wrong thing in bug 679065 (/report/list rather than the service call), so when evaluating the code again for 2.4 Josh and I both reviewed and didn't see why we should need to use an end-of-day function there.

Comment 5

[Robert Helmer [:rhelmer]]

lonnen, jberkus r? https://github.com/mozilla/socorro/pull/288

Comment 6

[[:jberkus] Josh Berkus]

Rob,

Looks fine.  You could either do what you did, or add one to the end_date; both actions are equivalent.

I will note that the whole approach in the tcbs screen is wrong; there's no good reason why we're querying the max date of tcbs instead of just using CURRENT_DATE.  That's something which dates back to the days when cron jobs used to fail every day.  However, we don't want to fix that as part of this bug.

So, +1 to merge.

Comment 7

[[github robot]]

Commit pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/4889f00dc0c29bedb01e27a4d8d57071b34c32c4
Merge pull request #290 from Lonnen/bug718901

bug 7108901 - max(report_date) from tcbs should be treated as end-of-day, not beginning