Skip to content

Introduce new handlers and refactor filters#37

Open
emmuhamm wants to merge 15 commits intodevelopfrom
emmuhamm/new-handlers
Open

Introduce new handlers and refactor filters#37
emmuhamm wants to merge 15 commits intodevelopfrom
emmuhamm/new-handlers

Conversation

@emmuhamm
Copy link
Contributor

@emmuhamm emmuhamm commented Jan 21, 2026

Description

Fixes #36

This PR Introduces a couple of changes that fixes #36

Added Throttling Filter (and demonstration)

  • This dynamically filters out messages if many of them get sent. Logic exactly follows the C++ implementation. Defaults are also the same, with 30 seconds and 30 messages. Note that when adding the Throttle filter using get_daq_logger, it uses a boolean and so the 30s and 30msg can't be changed. This is fine for now but should be easy to make more customisable later.

Added support for Lstdout

  • In ERS C++, Lstdout is the thread-safe version of stdout. The default logging implementation in Python is poggers as stdout is already threadsafe. Therefore, for this implementation we match HandlerType.Lstdout to just run for the stdout handler.
  • This is done via stdout_handler.addFilter(HandleIDFilter([HandlerType.Stream, HandlerType.Lstdout])).
  • Relevant changes in the code so HandleIDFilter supports multiple handlertypes to transmit with

Improves Filtering class logic and inheritance

  • Refactored the old HandleIDFiltering code to a BaseHandlerFilter class which contains a lot of the checks on when to emit a message or not.
  • Inherited by HandleIDFilter, makes it much easier to understand
  • Also Inherited by ThrottleFilter as Throttle is technically a 'HandlerType', to reuse the checking logic

Move away from string based log level matching to enum-based log level matching

  • When developing, there was a bug where using StreamHandler would break the ERS functionality.
  • This is because running StreamHandler modifies the record via the LoggingFormatter, and previously the Filter uses level_to_ers_var.get(record.levelname)), which would now contain erroneous whitespace and matching would fail
  • Moved from "DEBUG" to logging.DEBUG fixes this

Improves logging of the logging framework

  • in OKS, the ERS variables are of the form "erstrace,throttle,lstdout"
  • ERSTrace is no longer supported, so a debug message is sent via a logger in daqpytools to reveal this
  • If the variable is "erstrace,throttle,random", where 'random' is not supported, the logger will send a warning message.

TODO

  • Fix the comments @emmuhamm left in the code
  • Run linter

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature or enhancement (non-breaking change which adds functionality)
  • Optimization (non-breaking change that improves code/performance)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Testing checklist

  • Unit tests pass (e.g. dbt-build --unittest)
  • Minimal system quicktest passes (pytest -s minimal_system_quick_test.py)
  • Full set of integration tests pass (daqsystemtest_integtest_bundle.sh)
  • Python tests pass if applicable (e.g. python -m pytest)
  • Pre-commit hooks run successfully if applicable (e.g. pre-commit run --all-files)

How to test the changes

  • Check out this branch on the latest nightly
  • Run:
    • daqpytools-logging-demonstrator -r
      • Should see the normal messages
    • daqpytools-logging-demonstrator -r --throttle
      • Demonstration of throttling. Should see a bunch of message print out and then throttled, a 30s wait, and then more messages and throttling
    • daqpytools-logging-demonstrator -s --handlertypes
      • this enables the stream handler (stdout + stderr)
      • Should see the usual example messages, and then the ERS messages going through
      • You should see anything that has lstdout be printed via the stdout handler
      • NOTE there will be duplicates anytime stdout + stderr is used in the demonstrator (eg in the example messages). This is documented in [Bug]: StreamHandler log levels are weirdly being overridden #44, and beyond the scope of this PR.

Further checks

  • Code is commented where needed, particularly in hard-to-understand areas
  • Code style is correct (dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)
  • If applicable, new tests have been added or an issue has been opened to tackle that in the future.
    (Indicate issue here: # (issue))

@emmuhamm emmuhamm self-assigned this Jan 21, 2026
@emmuhamm emmuhamm force-pushed the emmuhamm/new-handlers branch from 5a05edb to 4a16756 Compare January 22, 2026 09:28
Base automatically changed from emmuhamm/introduce-ers to develop February 2, 2026 14:53
@emmuhamm emmuhamm force-pushed the emmuhamm/new-handlers branch 2 times, most recently from 21222c2 to f1d4953 Compare February 2, 2026 16:20
@emmuhamm emmuhamm force-pushed the emmuhamm/new-handlers branch from f1d4953 to a6ec6c9 Compare February 2, 2026 16:26
add_ers_kafka_handler(logger, use_parent_handlers, "session_tester")

if throttle:
# Note: Default parameters used. No functionality on customisability yet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discuss if we want to make it customizable here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion, it is decided to leave as is for now. Can discuss with Marco later on to get his opinion though

@emmuhamm emmuhamm changed the title Emmuhamm/new handlers Introduce new handlers and refactor filters Feb 3, 2026
from daqpytools.logging.levels import level_to_ers_var, logging_log_level_to_str
from daqpytools.logging.utils import get_width

class FormattedRichHandler(RichHandler):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is moved from the bottom of the file to up here so I can initialise the daqpytools logger

@@ -58,13 +160,17 @@ class HandlerType(Enum):
File = "file"
Protobufstream = "protobufstream"
Lstdout = "lstdout"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should note that while Lstdout is defined, I have not defined Lstderr just yet as it isnt used in any of the OKS configs as far as I can see.

Should be easy enough to implement. Do you want me to?

@emmuhamm
Copy link
Contributor Author

emmuhamm commented Feb 4, 2026

Hi @PawelPlesniak, in the interest of timing and availability I'll ask for your review today before I leave. The overall functionality exists and works as expected.

Theres a few minor improvements here and there that I've commented about, but they shouldn't affect the functionality. Theres also a couple where I ask for input.

But yeah, wouldbe great if you can take a look at the code already + the functionality, and lmk what you think

@emmuhamm emmuhamm marked this pull request as ready for review February 4, 2026 14:03
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.

[Feature]: Implement new set of Handlers

1 participant