Enable adding custom LogStasher fields from apps #327
Conversation
|
I believe this works, but it would be nice if there was a way to test that it does in the specs. Is that possible? |
After actually trying & failling to do this, I don't think it's realistically possible to replicate the scenario this is configured for in a test case. The custom fields are added via LogStasher, which uses log subscription callbacks to attach to Rails apps, and Thread.current values to store the custom fields - which means that to test this, we'd need to couple very tightly to LogStasher implementation. While LogStasher itself does something to address this in its own specs, I don't think it's right for us to do it here. |
|
Thanks for the reviews 🙇 I'll resolve the merge conflict, bump the gem a minor version number (in an additional commit) and merge this when I'm back at work on Monday |
|
I eventually found a way of testing this that's only medium-awkward. I've submitted it as a separate PR (#330) based off this branch, so you can accept or not as you see fit. |
I.e. allow apps to add their own custom fields in addition to those that `GovukJsonLogging` already adds. It looks like `LogStasher.add_custom_fields` can only be called one time, otherwise subsequent calls overwrite previous ones. We found this happening in the wild in the Content Store app, where `govuk_request_id` et al were missing from the Rails framework logs but present whenever the logger had been invoked directly from our own code. I am curious about the possibility of being able to add custom fields to both types of logs, together or separately, but the bug we're currently experiencing is just a conflict between two calls to `LogStasher.add_custom_fields`, so that's what I'm addressing. --- I've manually tested this change locally with Content Store, both with and without its own custom field config.
The way LogStasher hooks into Rails makes it difficult to test that configured custom fields actually get added. It was tricky to find an approach to worked, wasn't too awkward and wasn't too tightly coupled to LogStasher. _Notes on some of the esoteric details of these new tests_ `append_info_to_payload`: This is a Rails instrumentation hook (intended for use by third parties). It's run during request handling and LogStasher uses it to "append" its default fields and any custom fields defined by our app. The visible effect in the context of this method and its caller is that those LogStasher fields are set on the `payload` hash that was supplied to the method. So, invoking this method is enough to simulate a controller action running for LogStasher's purposes. And, if the payload hash contains our custom field key (and value, when there is one), we've defined it successfully. `logstasher_add_custom_fields_to_payload`: If a Rails app has defined custom LogStasher fields (with `LogStasher.add_custom_fields`), LogStasher registers a listener with Rails that fires whenever a controller is loaded and that listener adds this method to the loaded controller. Therefore, if a newly-loaded controller has this method, one or more LogStasher custom fields have been defined successfully.
mike29736
force-pushed
the
enable-adding-more-custom-logstasher-fields
branch
from
4fa7abc
to
2305f46
Compare
I.e. allow apps to add their own custom fields in addition to those that
GovukJsonLoggingalready adds.It looks like
LogStasher.add_custom_fieldscan only be called onetime, otherwise subsequent calls overwrite previous ones.
We found this happening in the wild in the Content Store app, where
govuk_request_idet al were missing from the Rails framework logs butpresent whenever the logger had been invoked directly from our own code.
This approach does solve the problem for Content Store (along with a
change to Content Store to actually use this option), but I'm keen to hear
suggestions of alternatives.
(https://trello.com/c/jDSDGNyn/814-measure-and-record-our-publishing-latency-sli)