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

DropEvent does not work in foreign_pre_chain #113

Open
gilbsgilbs opened this issue May 7, 2017 · 3 comments
Open

DropEvent does not work in foreign_pre_chain #113

gilbsgilbs opened this issue May 7, 2017 · 3 comments
Labels

Comments

@gilbsgilbs
Copy link
Contributor

gilbsgilbs commented May 7, 2017

Steps to reproduce

  1. Take this example from the doc.
  2. Append a processor that raises DropEvent in pre_chain.
  3. Run the example.

Expected behaviour

The log should be skipped.

Actual behaviour

Raises DropEvent; The exception is not caught.

Comments

I don't know if this is really expected or not. There should be at least a disclaimer in the documentation that states clearly that you shouldn't throw DropEvent in pre_chains processors, and you should instead write a stdlib Filter.

However, I reckon there is a way to handle the DropEvent in pre_chain. We could declare a custom Handler that proxies everything to the StreamHandler (or whatever handler is configured), except that we catch/ignore the DropEvent exception in the overidden emit method. This solution implies a tiny overhead on each log, since we have to forward each log entry to the underlying handler.

gilbsgilbs added a commit to gilbsgilbs/structlog that referenced this issue May 8, 2017
@hynek hynek added the stdlib label Apr 2, 2024
@mdgilene
Copy link

Based upon the closed PR, it seems like this is a "wontfix" issue? If so I still don't see any documentation about ProcessorFormatter not handling DropEvent.

@hynek
Copy link
Owner

hynek commented Sep 28, 2024

I wouldn't call it a wontfix, but the approach in the PR added too much complexity/moving parts. I would love to support this, but it would have to be a simpler approach since the stdlib integration already is a huge pain to maintain and use. Of course, I don't know if such an approach exists.

@mdgilene
Copy link

It seems like the root of the issue stems from the fact that ProcessorFormatter is implemented as a logging.Formatter. Thus, it has no ability to make decisions about whether a record should be logged at all. In the PR is a suggestion that seems like the most straightforward approach which would be to deprecate the ProcessorFormatter in favor of a ProcessorHandler which can handle everything the formatter was doing but the ability to drop the record entirely if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants