bug: add secondary_write for channels

[jrconlin]

• also set the default write_to_secondary to true

Closes SYNC-4121

[jrconlin]

There are a few extra debug and trace bits in there to help me identify a problem with my local testing.

[pjenvey]

couple things:

• save_message and save_messages are missing the metrics on ignoring the secondary error
• another thing we should fix while we're in here: get_channels has an inaccurate comment about having to copy over channels, this is probably from before we added the add_channels copying in get_user. Also I would argue that it's unique in that it doesn't need to check secondary at all, ever? So it would probably make sense to remove that block entirely? (It certainly shouldn't need to check secondary when channels.is_empty() I would more strongly argue)

[pjenvey]

• another thing we should fix while we're in here: get_channels has an inaccurate comment about having to copy over channels, this is probably from before we added the add_channels copying in get_user. Also I would argue that it's unique in that it doesn't need to check secondary at all, ever? So it would probably make sense to remove that block entirely? (It certainly shouldn't need to check secondary when channels.is_empty() I would more strongly argue)

though I suppose we could always have an edge case where node A migrates the user record, then node B reads it and the channels from bigtable before node A migrates the channels.. (even though bigtable keeps it all in the same row we write it separately)

[jrconlin]

* save_message and save_messages are missing the metrics on ignoring the secondary error

Both of those return DbResult<()> so we're passing the errors up. I figured we don't need to capture metrics for those because we're not eating the errors.
We are eating the Ok, but then it's just returning () anyway.

[jrconlin]

though I suppose we could always have an edge case where node A migrates the user record, then node B reads it and the channels from bigtable before node A migrates the channels..

That comment and the code pre-date your change to the channels, so it's fair to just drop it. I think the edge case is fairly minor and at worst would result in the client rebuilding it's list (if it's mobile, at least). We might also hit an equally edge case of an endpoint fetching the User Record and not finding a channel, then returning a 404 response, but that may also be temporary since we know providers don't generally pay attention to return codes.

[pjenvey]

Both of those return DbResult<()> so we're passing the errors up. I figured we don't need to capture metrics for those because we're not eating the errors.
We are eating the Ok, but then it's just returning () anyway.

ah, my mistake 👍