search

opensafely-core / ehrql

ehrQL: the electronic health record query language for OpenSAFELY
https://docs.opensafely.org/ehrql/
Other
7 stars 3 forks source link

fix: remove structlog and just use stdlib logging #2203

Closed bloodearnest closed 2 weeks ago

bloodearnest commented 2 weeks ago

We were not using most of the features of structlog in ehrql log outputs. The two we were using were coloured output, which we think we want to remove anyway, as it causes issue using logs in productions. The other was k=v formatted extra values, which we only use in one place.

stdlib logging can do the same job, and we just include the k=v tags a bit more manually when needed. This removes a dependency, which is always nice, but also means that the ehrql sandbox can work without any external dependencies, which is quite a nice property.

We may end up reintroducing coloured outputs again, but hopefully not by embedding ANSI colour codes in log files.

We also tweak the formatting, since we can do more easily now, removing timestamps and some whitespace.

cloudflare-workers-and-pages[bot] commented 2 weeks ago

Deploying databuilder-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5565ec5
Status: ✅  Deploy successful!
Preview URL: https://da7d661a.databuilder.pages.dev
Branch Preview URL: https://remove-structlog.databuilder.pages.dev

View logs

evansd commented 2 weeks ago

@bloodearnest Oh, one more thing. Given that this involves user-facing changes we'll want a new release so one of the commits needs a pedantic prefix.

bloodearnest commented 2 weeks ago

I'm really not a fan of gh's default thing of repeating the commit titles as bulletpoints.

Yeah, its not great.

But I'm not a huge fan of the details of motivation being in the merge commit and not the commit that added the change.

evansd commented 2 weeks ago

I'm not a huge fan of the details of motivation being in the merge commit and not the commit that added the change.

Having it in both is great! But the Github UI makes it pretty easy to jump from a commit to the PR which merged it, and there's always going to be context there which isn't available on the commit itself (review comments etc.) so if it is only going to be in one place I'd prioritise the PR body.

evansd commented 2 weeks ago

BTW, the pedantic prefix needs to be on a commit rather than the PR title (because its pedantry knows no bounds).

bloodearnest commented 2 weeks ago

bah, I thought I had prefixed both the commit and the PR title with fix: but it looks like I hadn't pushed the edited commit message