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

Chore: RabbitMQ logging #414

Merged
merged 6 commits into from
Sep 11, 2023
Merged

Chore: RabbitMQ logging #414

merged 6 commits into from
Sep 11, 2023

Conversation

kiwiidb
Copy link
Contributor

@kiwiidb kiwiidb commented Sep 7, 2023

Fixes #411
This PR adds metadata fields to the logs from the processes that use Rabbitmq: payment consumer, invoice consumer and publisher routines. This will help us identify issues and also allow us to trace payments/invoices across services.

I also propose a standardized way of naming metadata fields (I would propose snake case), so we can search for them in datadag across different service logs:

  • payment_hash (only one used in this PR)
  • lndhub_user_id, getalbycom_user_id
  • invoice_id
  • message
  • error
  • etc

We would need some follow up PR's in order to implement this kind of structured logging across LNDhub & other services.

@kiwiidb kiwiidb requested a review from bumi September 7, 2023 12:48
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #414 (f973c3e) into main (018a1b3) will decrease coverage by 0.35%.
Report is 2 commits behind head on main.
The diff coverage is 41.05%.

❗ Current head f973c3e differs from pull request most recent head b929ca3. Consider uploading reports for the commit b929ca3 to get more accurate results

@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   59.12%   58.77%   -0.35%     
==========================================
  Files          51       51              
  Lines        3481     3556      +75     
==========================================
+ Hits         2058     2090      +32     
- Misses       1202     1245      +43     
  Partials      221      221              
Files Changed Coverage Δ
rabbitmq/rabbitmq.go 52.59% <40.42%> (-3.39%) ⬇️
controllers_v2/invoice.ctrl.go 14.78% <100.00%> (+0.74%) ⬆️

Copy link
Contributor

@bumi bumi left a comment

Choose a reason for hiding this comment

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

good idea with the subroutine field!

func captureErr(logger *lecho.Logger, err error) {
logger.Error(err)
func captureErr(logger *lecho.Logger, err error, j log.JSON) {
j["error"] = err
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this adding to the logging? the error message?

logger.Error(err)
func captureErr(logger *lecho.Logger, err error, j log.JSON) {
j["error"] = err
logger.Errorj(j)
sentry.CaptureException(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

when I currently look at the sentry stack traces it does not give much information and it does not contain the original stack trace but links to this CaptureException error

I read here something about a different erros package and wrapping an error to keep the stack trace?
https://incident.io/blog/golang-errors
does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go stacktraces inherently require more work, yes. So that would be something to add later.

"subroutine": "invoice publisher",
"message": "succesfully published invoice",
"payment_hash": invoice.RHash,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

is some more metadata helpful like the key (and exchange?)
the key is computed it might be interesting to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exchange is always the same here but key can be added.

@kiwiidb kiwiidb merged commit 7a5a778 into main Sep 11, 2023
3 checks passed
@kiwiidb kiwiidb deleted the chore/payment-finalizer-logging branch September 11, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JSON logging to rabbitmq publishing
2 participants