Skip to content

Reduce log level of failure to publish report to DB#1924

Merged
awygle merged 3 commits intomobilecoinfoundation:masterfrom
awygle:GH1892
Jun 6, 2022
Merged

Reduce log level of failure to publish report to DB#1924
awygle merged 3 commits intomobilecoinfoundation:masterfrom
awygle:GH1892

Conversation

@awygle
Copy link
Contributor

@awygle awygle commented May 5, 2022

Soundtrack of this PR: The Impressions - It's All Right

Motivation

Bringing forward changes which were in progress before moving the fog repo to the unified repo.

In this PR

  • Reduce log level of failure to publish report to DB

Context from [mobilecoinfoundation/fog#147]:

Sometimes, we lose connection to the sql DB, and the report server write operation fails.

This should not be logged as an error because it's not actionable and errors get converted to sentry alerts.

Fixes [mobilecoinfoundation/fog#147] and [#1892].

@awygle awygle requested review from cbeck88 and jcape May 5, 2022 21:50
@awygle awygle marked this pull request as ready for review May 5, 2022 21:50
Copy link
Contributor

@iamalwaysuncomfortable iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing the comment PublishError catch in favor of a shorter comment above the catch (which would be more rust-idiomatic) would be the simplest way to improve the PR.

A more elegant way might be to split the publisher error in the PR into two more contextual errors and do the log::warn! about the publisher error within the Error::PublishReport branch of the error.

Something to the tone of:

    /// Incorrect ingress key: {0}
    BadIngressKey(CompressedRistrettoPublic)
    /// Report publication/ingress key checkup failed: {0}
    PublishReport(String),

This more detailed way is entirely optional though as the FOG SLA is improved either way.

Copy link
Contributor

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should keep sleeping when PublishReport error occurs, as we did before

@iamalwaysuncomfortable
Copy link
Contributor

Bump to @awygle on this - what are your thoughts on how to proceed forward? This is a positive FOG SLA item 🥇 that I think is almost in reach.

@awygle awygle dismissed cbeck88’s stale review June 6, 2022 17:20

Addressed in subsequent commit.

@awygle awygle merged commit 0e48d5a into mobilecoinfoundation:master Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants