Use sp_return_logs for billing reports, not monthly table

[zachmargolis]

(see context in Slack discussion)

Accuracy

We caught a few discrepancies in our reports

• the monthly table is off in some some cases, I couldn't get it to match sp_return_logs, so since sp_return_logs is the raw table, it seemed more accurate and easier to check if both reports used the same thing
• when we use BETWEEN '2021-01-01' AND '2021-01-31' we get truncated results compared to the fuller timestamp version BETWEEN '2021-01-01 00:00:00.00000' AND '2021-01-31 23:59:59.999999' so switches this to use that

Performance

sp_return_logs is a bigger table, so these queries will likely be slower. I am running them now to make sure they return eventually, but unfortunately I think we need to be making this performance/accuracy tradeoff right now.

Next Steps

• Apparently billing only uses these two reports, so maybe we should remove all the rest
• Maybe we drop the monthly tables since we have trouble reconciling them, and we still retain the raw data

[zachmargolis]

technically these are ActiveSupport::TimeWithZone ... but that was a lot more to write, and that's a subclass, so I felt like the gist was correct here

[zachmargolis]

see commit notes (70ae9c6) for a longer comment, but short version is we get these errors occasionally and I figured it was worth a shot seeing if we could quickly retry + recover rather than abort the entire job

[zachmargolis]

and in case it's not obvious what it's doing, we have a ruby nested hash/multiset thing object where we add results incrementally and this is creating a copy before streaming the results, and then restoring the copy to the last known good result every time the query fails

we could rewrite this as begin/rescue syntax but we'd need add some sleep code and need a retry counter ourself, so this seemed clear enough? the alternative would be:

temp_copy = ial_to_year_month_to_users.deep_dup
attempt_count = 0
begin
  stream_query(query) do |row|
    # ...
  end
rescue PG::TRSerializationFailure => e
  attempt_count += 1
  if attempt_count <3
    ial_to_year_month_to_users = temp_copy
    retry
   else
    raise e
  end
end

[zachmargolis]

now that we're using the direct table, this was just to create fewer rows and make a faster test

[zachmargolis]

I think this is correct now that this has another section... I think it may have been grouped inaccurately before

[zachmargolis]

so I think that this change may mean these are essentially unindexed queries now 😬 , this is our partial index:

identity-idp/db/schema.rb

Line 588 in c62f569

t.index "((requested_at)::date), issuer", name: "index_sp_return_logs_on_requested_at_date_issuer", where: "(returned_at IS NOT NULL)"

cc @mitchellhenke @stevegsa @jmhooper if you have any thoughts on if I should try to add a "plain" index on (requested_at, issuer)? (and it's a huge table so I know it would fail during a normal deploy)

[stevegsa]

Yes we need requested_at and issuer does nothing it can be dropped.

[zachmargolis]

for the combined billing reports, we do break things out by issuer by month, so I think it does help for those?

[zachmargolis]

update, thanks to some investigation help from @mitchellhenke, the ::date truncation does work like we expect so I undid this timestamp change part in c5b005e

[stevegsa]

looks great. just need the index and i think we are good to go. not sure why date isn't getting that equivalent full timestamp though. if it was this pr wouldn't be needed correct?