Add govuk_request_id to logs when available #1202
Conversation
Whenever an error is raised by the api library, the output from the api client now contains govuk_request_id whenever its available.
|
I'd be somewhat cautious about logging all We also probably don't want to log new headers added in the future without anyone even necessarily knowing that that'll happen. So I think explicit is the safer way to go here. Also helps to keep the logs tidy and usable if we add stuff as needed rather than speculatively. |
I get what you mean and would agree if it was regular info/warn logging, however this is for debug level logging so I'd expect pretty much everything at that granularity - nor would I expect people to be running that in prod by default. |
|
Ah ok, that's fair. From the PR description it doesn't look like debug logging though. |
|
The screenshot is from Logit.io in fact. |
|
@tunylund I'd recommend keeping the scope of this as it is. If there's a need to log other headers then folks can easily add them as/when needed. There's really no upside to logging stuff that we don't know we need, but there's a real downside risk which is it could cause another privacy incident and that'd be a real pain. |
|
Ah you're right, it is only debug on the line in the preview the others are warn and error. Sorry my bad As this uses the Rails logger, does this https://github.com/alphagov/govuk_app_config/blob/e5edc331726e1a0668c50eff53646d2b8534dc47/lib/govuk_app_config/govuk_json_logging.rb#L20-L22 not also add this in? |
Unfortunately not. It would be nice if Rails.logger would be used, but in order to do that, we would have to implement the |
Oh sure, I didn't even know we could use logstasher as just a regular logger . Yeah seems reasonable about the JSON. Of course there had to be a second usage of JSON logging hidden away. If we find more maybe we'll have to revisit the JSON.parse but hopefully not as seems just more layers of complexity. |
|
Sorry - closed by mistake |
|
I merged this already, but just realized, if we would implement JSON.parse into Rails.logger, then we wouldn't need this pr at all. |
Sure I can understand that though I still think it's best that that isn't the approach myself - particularly given that if a field is already used in logit with a different format the log will just get lost. It makes me feel quite uneasy having such free form log structure when the consequences of a naming collision is quite severe. I imagine it'd be nicest if we had just a limited set of root level fields and, for examples like this where you want to log JSON, an ability for that to be logged under a namespace (i.e. for this gds_api_adapters) so that it wasn't just putting everything at root fields ready to collide. |
|
Yeah it's a real pain that ELK is so fussy about schemas; it'd be so much less of a hassle if it erred on the side of accepting whatever's thrown at it and just warning that it doesn't match the schema or whatever. But we've mostly just gotta live with that. We could probably improve things a bit by trying to align with the common schema (if you can't beat 'em, join 'em), but it's one of those things that could sink a lot of up-front effort for little real-world gain — so maybe we're better off just fixing these things as we encounter real-world use cases like we've been doing? 🤷 |
Ooh I didn't know about that common schema - thanks for sharing. I guess we've got that by default applied, as looking that error one in that explains where the schema appeared that nuked our field that was setting error as a string. Would agree on time vs gain - would certainly be nice and elegant but not really impact any person's workflow (and probably have a whole bunch of stuff that is hard to slot in). The thing that bugs me about it is how hard it is it fix a case if something is wrong. Seems once you've got misindexed field type you've got to be prepared to throw the whole lot in the bin to change it 🙀 |
|
The schemas are versioned so at least in theory it should never force you to delete old logs just to make a schema change, but it certainly drops stuff on the floor on ingestion if it doesn't perfectly match what it's expecting :( |
This pr aims to fix an issue where the gds-api-adapters logs output whenever an error has happened but the output is missing
govuk_request_idWhenever an error is raised by the api library, the output from the api client now contains govuk_request_id whenever its available.
gds-api-adapters by default sets up a header sniffer for govuk_request_id here: https://github.com/alphagov/gds-api-adapters/blob/main/lib/gds_api/railtie.rb#L7 which allows the code below to just add the header value into log lines.
I looked into if we could make govuk_app_config add the header to these log lines as it does something similar for the default Rails logger, but it looks like the most straightforward way is this.