Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(py3): Bump structlog #19877

Merged
merged 1 commit into from Jul 15, 2020
Merged

Conversation

evanpurkhiser
Copy link
Member

@evanpurkhiser evanpurkhiser commented Jul 15, 2020

Version 17.1.0 is the first to support python3. We were on 16.1, this is a single release bump.

Notable changes:

Pretty much all of the other changes are documentation changes, or minor code changes.

Overall this bump should be trivial.

@evanpurkhiser evanpurkhiser requested a review from a team as a code owner July 15, 2020 00:31
@evanpurkhiser evanpurkhiser marked this pull request as draft July 15, 2020 00:31
@joshuarli
Copy link
Member

There's barely any coverage. Only tests/sentry/logging/test_handler.py which doesn't do much.

We seem to have customized structlog pretty deeply... have a look at configure_structlog. We use simplejson in our own JSONRenderer, and simplejson vs stdlib json is an entirely separate python3 consideration, so I think bumping structlog is going to need some more investigation and possibly we should block this with simplejson vs stdlib json.

FWIW there is a https://www.structlog.org/en/stable/api.html#structlog.processors.JSONRenderer, I don't know if there's a reason we roll our own. Maybe @JTCunning can shed some light on that one.

@joshuarli
Copy link
Member

Other considerations include:

# These are values that come default from logging.LogRecord.
# They are defined here:
# https://github.com/python/cpython/blob/2.7/Lib/logging/__init__.py#L237-L310
throwaways = frozenset(
    (
        "threadName",
        "thread",
        "created",
        "process",
        "processName",
        "args",
        "module",
        "filename",
        "levelno",
        "exc_text",
        "msg",
        "pathname",
        "lineno",
        "funcName",
        "relativeCreated",
        "levelname",
        "msecs",
    )
)

@evanpurkhiser
Copy link
Member Author

Yeah I'm already reading all of this and talking with James :)

@evanpurkhiser
Copy link
Member Author

possibly we should block this with simplejson vs stdlib json.

I think there's no problem bumping structlog to a known python3 compatible version (which is really all this is, since the version bump just brings us to a version where py3 tests are running). When we bump simplejson we can circle back to this and make sure things look OK.

@joshuarli
Copy link
Member

possibly we should block this with simplejson vs stdlib json.

I think there's no problem bumping structlog to a known python3 compatible version (which is really all this is, since the version bump just brings us to a version where py3 tests are running). When we bump simplejson we can circle back to this and make sure things look OK.

Yeah, I agree.

@evanpurkhiser evanpurkhiser marked this pull request as ready for review July 15, 2020 19:28
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

This looks safe to me. Browsing through their changes it's mostly docs and things that shouldn't affect us

@evanpurkhiser evanpurkhiser merged commit 91615dd into master Jul 15, 2020
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/chorepy3-bump-structlog branch July 15, 2020 20:54
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants