Skip to content

Refactor logging helpers to reduce signature creep

Matthias Käppler requested to merge mk/refactor-logging into master

Overview

I was not happy with the existing logging/response helpers since they are not flexible enough. There are too many variations that combine different contextual parameters to log or capture errors. It was also inconsistently applying correlation ID and common request fields (these were only added to error logs, not info logs.)

I rewrote those functions using the builder pattern, just like the underlying library does, so as to have a flexible and extensible "DSL" that does not repeat itself.

Examples:

// Just log something
log.Info("hello")

// Send error to Sentry and print it using ERROR level
log.Error(err)

// Send error to Sentry and print it using ERROR level with custom fields
log.WithFields(customFields).Error(err)

// Using request fields plus custom fields and print message using INFO level
log.WithRequest(r).WithFields(customFields).Info("success")

Approach

Jacob suggested to de-risk this MR (given its blast radius) by first focusing only on migrating the API while still calling into the existing implementation (akin to a service strangler.), which I thought was a great idea. Then we would be able to swap out the impl in a subsequent MR. That turned out to be difficult to do in practice though, due to the nature of the API: it's a builder, so it separates configuration from performing the logging side-effect. It means that all the interesting logic would still fire at once, when the builder is "terminated". So nothing that made the builder a builder in the first place would be testable with this separation.

However, I left all code intact for now that uses the "log and fail" logic (Fail500, CaptureAndFail etc). I had initially also migrated all of in this MR but reverted those changes and will spin that out into a separate MR. That way, once both changes are in place, the helper module will continue to offer a Fail500 and similar functions, but it will be based on this new logging API internally.

labkit compat

Eventually this should likely live in labkit, but I'd prefer for APIs to mature in actual projects before being extracted into shared libraries. I do not know if Gitaly or GL-Runner have the same or similar requirements in terms of logging interfaces. Meanwhile, I have tried to remain as close as possible to logrus and labkit APIs to make this extraction simple if desired.

Next steps:

  • rework helpers to be expressed in terms of this new builder API and streamline the use and signatures of FailXXX helpers that both log and fail
  • migrate to labkit's Tracker interface for error tracking
Edited by Matthias Käppler

Merge request reports