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

Add LogfmtRenderer #376

Merged
merged 9 commits into from
Dec 5, 2021
Merged

Add LogfmtRenderer #376

merged 9 commits into from
Dec 5, 2021

Conversation

airwoodix
Copy link
Contributor

Summary

This PR adds a LogfmtRenderer processor to format log lines in the logfmt format.

The core of the work is done by the logfmt library, which is added as global dependency. Or should this be changed to feature-based optional?

Refactored the code in KeyValueRenderer to share the sort_keys/key_order/drop_missing logic. Tests are directly adapted from KeyValueRenderer.

Closes #322

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do.

  • Added tests for changed code.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
  • Changes (and possible deprecations) are documented in the changelog.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Copy link
Owner

@hynek hynek left a comment

Choose a reason for hiding this comment

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

Wow, this is great thank you! logfmt doesn't seem to be too well-maintained these days, but we can give it a try before we implement it ourselves.

As you've suspected, we need to make logfmt optional, I've added instructions to the pyproject.toml lines

Tasks that I couldn't add to the diff:

  • add test env with logfmt to tox.ini (analogous to the rich env)
  • add a usage example to typing_examples.py

CHANGELOG.rst Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/structlog/processors.py Outdated Show resolved Hide resolved
src/structlog/processors.py Outdated Show resolved Hide resolved
src/structlog/processors.py Show resolved Hide resolved
src/structlog/processors.py Outdated Show resolved Hide resolved
src/structlog/processors.py Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
src/structlog/processors.py Show resolved Hide resolved
tests/test_processors.py Show resolved Hide resolved
@hynek
Copy link
Owner

hynek commented Dec 4, 2021

Um, so I've had a look at logfmt (of course one second after sending the review 🤦‍♂️) and I don't think we need a dependency for 17 LoC?

def format_line(extra):
    outarr = []
    for k, v in extra.items():
        if v is None:
            outarr.append("%s=" % k)
            continue


        if isinstance(v, bool):
            v = "true" if v else "false"
        elif isinstance(v, numbers.Number):
            pass
        else:
            if isinstance(v, (dict, object)):
                v = str(v)
            v = '"%s"' % v.replace('"', '\\"')
        outarr.append("%s=%s" % (k, v))
    return " ".join(outarr)

@airwoodix
Copy link
Contributor Author

Thanks for the fast and detailed review!

I don't think we need a dependency for 17 LoC?

Agreed. I added the logfmt rendering logic to LogfmtRenderer.__call__ in 44a19e6.

Since there's no formal specification, I oriented myself to the behavior of the go and node implementations. The go docs have an example and a pseudo-spec.

I also quickly checked that Grafana's LogQL logfmt filter/parser is happy with quoted/unquoted strings, empty values (key=) and boolean flags (key).

add test env with logfmt to tox.ini (analogous to the rich env)

I guess that this is obsolete with the dependency to logfmt removed?

add a usage example to typing_examples.py

I added a block similar to the ones for JSONRenderer in f3b5ffe, though I'm not sure how the typing_examples.py mechanism works.

@airwoodix airwoodix requested a review from hynek December 4, 2021 21:50
@hynek
Copy link
Owner

hynek commented Dec 5, 2021

Thanks for the fast and detailed review!

Thanks for the quick turnaround!

I don't think we need a dependency for 17 LoC?

Agreed. I added the logfmt rendering logic to LogfmtRenderer.__call__ in 44a19e6.

Since there's no formal specification, I oriented myself to the behavior of the go and node implementations. The go docs have an example and a pseudo-spec.

Wonderful, looks great! We might want to add a default mechanism like json has, but good enough start!

add test env with logfmt to tox.ini (analogous to the rich env)
I guess that this is obsolete with the dependency to logfmt removed?

yep

add a usage example to typing_examples.py
I added a block similar to the ones for JSONRenderer in f3b5ffe, though I'm not sure how the typing_examples.py mechanism works.

it just uses the APIs and looks whether mypy is happy or not. the code is never executed.

src/structlog/processors.py Show resolved Hide resolved
@hynek hynek merged commit 07e06a4 into hynek:main Dec 5, 2021
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.

Logfmt formatter is missing
2 participants