Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add log file for important notifications #1982

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Oct 1, 2021

Add a new log file for important notifications that require an action from the node operator.

Using a separate log file makes it easier than grepping specific messages from the standard logs, and lets us use a different style of messaging, where we provide more information about what steps to take to resolve the issue.

We rely on an event sent to the event stream so that plugins can also pick it up and connect with notification systems (push, messages, mails, etc).

@t-bast t-bast requested a review from pm47 October 1, 2021 13:18
@akumaigorodski
Copy link
Contributor

Can this be made catchable by AlarmBot plugin (maybe publish some kind of event to stream)? This use case fits it like a glove.

@t-bast
Copy link
Member Author

t-bast commented Oct 1, 2021

Can this be made catchable by AlarmBot plugin (maybe publish some kind of event to stream)? This use case fits it like a glove.

Yes definitely. I planned on creating an event, but wasn't sure how it would be used.
Is a case class NodeOperatorNotification(message: String) enough for your alarm bot?

@akumaigorodski
Copy link
Contributor

Is a case class NodeOperatorNotification(message: String) enough for your alarm bot?

Yes.

@t-bast t-bast force-pushed the node-operator-notifications branch from 25b0a2d to 30c9bd7 Compare October 1, 2021 15:56
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

Merging #1982 (78efe48) into master (498e9a7) will decrease coverage by 0.09%.
The diff coverage is 56.00%.

@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
- Coverage   87.73%   87.63%   -0.10%     
==========================================
  Files         159      159              
  Lines       12455    12471      +16     
  Branches      522      545      +23     
==========================================
+ Hits        10927    10929       +2     
- Misses       1528     1542      +14     
Impacted Files Coverage Δ
...n/scala/fr/acinq/eclair/balance/BalanceActor.scala 10.90% <0.00%> (-0.42%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.50% <0.00%> (-0.45%) ⬇️
...air-core/src/main/scala/fr/acinq/eclair/Logs.scala 82.75% <44.44%> (-7.04%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 88.08% <50.00%> (-0.13%) ⬇️
...lair/blockchain/watchdogs/BlockchainWatchdog.scala 61.53% <87.50%> (-2.57%) ⬇️
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 80.68% <100.00%> (+0.11%) ⬆️
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 93.10% <0.00%> (-3.45%) ⬇️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 95.93% <0.00%> (-1.63%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 86.20% <0.00%> (-1.15%) ⬇️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 84.16% <0.00%> (-0.42%) ⬇️
... and 1 more

Copy link
Member

@pm47 pm47 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 also send a notification when we detect bitcoin may be eclipsed.

@t-bast t-bast force-pushed the node-operator-notifications branch from c8fa2cb to 78efe48 Compare October 18, 2021 12:17
@t-bast
Copy link
Member Author

t-bast commented Oct 18, 2021

We should also send a notification when we detect bitcoin may be eclipsed.

Done in 78efe48

@pm47
Copy link
Member

pm47 commented Oct 22, 2021

Needs rebase, otherwise LGTM.

Add a new log file for important notifications that require an action from
the node operator.

Using a separate log file makes it easier than grepping specific messages
from the standard logs, and lets us use a different style of messaging,
where we provide more information about what steps to take to resolve
the issue.

We rely on an event sent to the event stream so that plugins can also pick
it up and connect with notification systems (push, messages, mails, etc).
@t-bast t-bast force-pushed the node-operator-notifications branch from 78efe48 to 72ba251 Compare October 25, 2021 08:18
@t-bast
Copy link
Member Author

t-bast commented Oct 25, 2021

Rebased

@t-bast t-bast merged commit 765a0c5 into master Oct 25, 2021
@t-bast t-bast deleted the node-operator-notifications branch October 25, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants