Skip to content

feat: Ability to log WSGI environ in JSON logs, log_extra utility#55

Merged
khvn26 merged 7 commits intomainfrom
feat/log-wsgi-extras
Apr 22, 2025
Merged

feat: Ability to log WSGI environ in JSON logs, log_extra utility#55
khvn26 merged 7 commits intomainfrom
feat/log-wsgi-extras

Conversation

@khvn26
Copy link
Copy Markdown
Member

@khvn26 khvn26 commented Apr 21, 2025

This PR capitalises on Gunicorn's existing ability to log request/response headers and WSGI environ keys, enabling our JSON access log formatter to log these as well.

Flagsmith will now accept an ACCESS_LOG_JSON_EXTRA_ITEMS setting, consisting of Gunicorn atom keys, e.g.:

  • {x-environment-key}i: the X-Environment-Key request header...
  • ...{content-type}o: the Content-Type response header...
  • ...{wsgi.version}e: the wsgi.version WSGI environ value...

...in line with the ACCESS_LOG_FORMAT setting we employ for rendering the non-JSON access logs.

The PR also exposes the common.gunicorn.utils.log_extra utility function which is a shortcut to logging the extra values as WSGI environ keys with the flagsmith. prefix. The custom key used for logging routes has been updated to match this convention.

This is part 1 of an effort to enable logging environment keys/ids in access logs for a major customer.

@khvn26 khvn26 requested a review from a team as a code owner April 21, 2025 18:58
@khvn26 khvn26 requested review from gagantrivedi and removed request for a team April 21, 2025 18:58
from common.gunicorn.utils import get_extra

_wsgi_extra_key_regex = re.compile(
r"^{(?P<key>[^}]+)}(?P<suffix>[%s])$" % "".join(WSGI_EXTRA_SUFFIX_TO_CATEGORY)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to perform this validation ourselves? If someone passes in a bad specification, it shouldn't be up to us to validate that IMO. This should blow up as early/violently as possible if something wrong was passed in.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do we need to perform this validation ourselves?

Unfortunately so as the atom templates are hard-coded in Gunicorn — the good news is they are part of user-facing API so very unlikely to change.

This should blow up as early/violently as possible if something wrong was passed in.

Currently, bad keys in the setting are just ignored by the logger. It's easy to make this pattern public and add validation in the main repo's common settings, though 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 95fc11b.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I meant to say is, if we can't make Gunicorn fail when using invalid templates, then we should respect its behaviour, even if that's silently doing nothing. In future Gunicorn versions this might change and it shouldn't be up to us to decide what the behaviour should be.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just to make sure we're on the same page, re.Pattern.match does not raise an exception when a match is not found.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's fine - my point is this code is not necessary, we don't need to validate things before they reach Gunicorn. It's up to users of this variable to pass the correct formats and ensure everything works as expected.

Copy link
Copy Markdown
Member Author

@khvn26 khvn26 Apr 21, 2025

Choose a reason for hiding this comment

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

Yeah, was just updating my comment to point out that the pattern is not used for validation, rather for convenient key/suffix extraction.

@khvn26 khvn26 requested a review from rolodato April 21, 2025 20:19
@khvn26 khvn26 merged commit bf48843 into main Apr 22, 2025
4 checks passed
This was referenced Jul 29, 2025
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