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

slog: update to use structured logs for hashmail server #148

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ellemouton
Copy link
Member

Update aperture's btclog and lnd deps so that it can take advantage of structure logging.

Then start using the structured logs for the hashmail server.

This will make querying logs much easier & faster.

Example

Main thing to note here is the uniformity of the "stream_id" attribute in the new logs.

Old:

Screenshot 2024-09-11 at 17 16 56

New:

Screenshot 2024-09-11 at 17 16 15

@guggero guggero self-requested a review September 11, 2024 16:07
@ellemouton ellemouton force-pushed the slog branch 3 times, most recently from 550bede to 50cbe19 Compare September 16, 2024 05:02
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice! And cool to see a demo of the context based trace functionality 💯

LGTM pending the dependent PRs and the err value fix.

hashmail_server.go Outdated Show resolved Hide resolved
log.DebugS(ctxl, "New HashMail stream init", "auth", init.Auth)

if err := h.ValidateStreamAuth(ctxl, init); err != nil {
log.DebugS(ctxl, "Stream creation validation failed", err)
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be "err", err, otherwise this would show as "bad key", because DebugS doesn't have an err parameter. Same further below.

Copy link
Member Author

Choose a reason for hiding this comment

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

a yes good catch!

makes me wonder if we should switch this to a "WarnS"? but anyways, keeping it as debug for now 👍

@ellemouton ellemouton force-pushed the slog branch 2 times, most recently from c329b86 to 2a10761 Compare October 23, 2024 09:48
Update the deps so that structured logging is availabel in aperture.
This will make querying by stream IDs more uniform (and hence, way
easier).
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM pending the dependencies 🎉

@@ -621,6 +629,9 @@ func (h *hashMailServer) SendStream(readStream hashmailrpc.HashMail_SendStreamSe
return err
}

ctx := btclog.WithCtx(readStream.Context(),
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is a normal (non-formatting) function call, so I think the other multi-line formatting rule would apply?

Same further below.

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.

2 participants