Repository: OpenAssetIO/OpenAssetIO (language: python)

Issue:
Add severity check to LoggerInterface
## What

Provide a mechanism for the middleware to ask the `LoggerInterface` if the message they are about to construct will be logged, so they can avoid constructing the string if not neccesary.

## Why

Constructing log strings can be expensive, especially in hot code paths.  Doing so only for the log severity level to be such that the string is never printed is suboptimal, to say the least.

See review conversation https://github.com/OpenAssetIO/OpenAssetIO/pull/1011#discussion_r1256109927

## Acceptance Criteria
- New method `isSeverityLogged(Severity)` added to `LoggerInterface`
- Default implementation for this method that returns true.
- Implement and obey this additional `LoggerInteface` flag in addition to current `maxSeverity` in `SeverityFilter`

Add severity check to LoggerInterface
## What

Provide a mechanism for the middleware to ask the `LoggerInterface` if the message they are about to construct will be logged, so they can avoid constructing the string if not neccesary.

## Why

Constructing log strings can be expensive, especially in hot code paths.  Doing so only for the log severity level to be such that the string is never printed is suboptimal, to say the least.

See review conversation https://github.com/OpenAssetIO/OpenAssetIO/pull/1011#discussion_r1256109927

## Acceptance Criteria
- New method `isSeverityLogged(Severity)` added to `LoggerInterface`
- Default implementation for this method that returns true.
- Implement and obey this additional `LoggerInteface` flag in addition to current `maxSeverity` in `SeverityFilter`



Discussion / hints:
Capturing some old discussion that just popped into my head. Now that we have libfmt it would be great if we could

- Make logger severity a first-class part of the core `LoggerInterface`
- Allow use of format strings in the `LoggerInterface` log methods

This then allows lazy evaluation and printing of log messages, potentially no-oping based on severity. 

But the problem is, in a naive implementation we'd have to make libfmt a public dependency, potentially clashing with a host application/plugin's bundled libfmt. This is because we'd have to use variadic templates.

We could vendor and re-namespace libfmt (I suspect we'll end up having to do this for some libraries eventually). But would be good to avoid that.

Another idea is to allow the logger methods to take as input a lambda that returns a string, and only evaluate it if the severity warrants it. But that's boilerplate.
Capturing some old discussion that just popped into my head. Now that we have libfmt it would be great if we could

- Make logger severity a first-class part of the core `LoggerInterface`
- Allow use of format strings in the `LoggerInterface` log methods

This then allows lazy evaluation and printing of log messages, potentially no-oping based on severity. 

But the problem is, in a naive implementation we'd have to make libfmt a public dependency, potentially clashing with a host application/plugin's bundled libfmt. This is because we'd have to use variadic templates.

We could vendor and re-namespace libfmt (I suspect we'll end up having to do this for some libraries eventually). But would be good to avoid that.

Another idea is to allow the logger methods to take as input a lambda that returns a string, and only evaluate it if the severity warrants it. But that's boilerplate.

Fix patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index f2b4ed379..bea5a48de 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -15,6 +15,14 @@ v1.0.0-beta.x.y
   a `CppPluginSystemManagerPlugin` object.
   [#1115](https://github.com/OpenAssetIO/OpenAssetIO/issues/1115)
 
+- Added `LoggerInterface.isSeverityLogged` to allow users to
+  short-circuit log message construction if the logger indicates the
+  intended severity will be filtered. Updated `SeverityFilter` to use
+  the most pessimistic of its own severity and that of the wrapped
+  logger.
+  [#1014](https://github.com/OpenAssetIO/OpenAssetIO/issues/1014)
+
+
 v1.0.0-beta.2.1
 ---------------
 
diff --git a/src/openassetio-core/include/openassetio/log/LoggerInterface.hpp b/src/openassetio-core/include/openassetio/log/LoggerInterface.hpp
index 4674843ef..44315bf27 100644
--- a/src/openassetio-core/include/openassetio/log/LoggerInterface.hpp
+++ b/src/openassetio-core/include/openassetio/log/LoggerInterface.hpp
@@ -50,13 +50,39 @@ class OPENASSETIO_CORE_EXPORT LoggerInterface {
    * This method must be implemented to present the supplied message
    * to the user in an appropriate fashion.
    *
-   * @param message The message string to be logged.
-   *
    * @param severity One of the severity constants defined in @ref
    * Severity.
+   *
+   * @param message The message string to be logged.
    */
   virtual void log(Severity severity, const Str& message) = 0;
 
+  /**
+   * Check if a given severity level should/will be filtered out.
+   *
+   * The implementation of the logger may have a mechanism by which
+   * certain severity levels are not output. If a severity level is not
+   * output, then constructing a string to pass to the logger is wasted
+   * effort. This method can be queried before constructing a complex
+   * string, in order to avoid that wasted effort.
+   *
+   * Implementors of LoggerInterface subclasses should override this
+   * method if they wish to conditionally skip logging at particular
+   * severity levels.
+   *
+   * If @ref log is called regardless, with a severity that elicits a
+   * `false` response from this method, then the logger may still output
+   * the message, but it is not guaranteed (and is discouraged).
+   *
+   * The default implementation returns `true` for all severities.
+   *
+   * @param severity Severity level to check.
+   *
+   * @return Whether a message will be output if `log` is called
+   * with the given severity.
+   */
+  [[nodiscard]] virtual bool isSeverityLogged(Severity severity) const;
+
   /**
    * @name Conveniences
    * @{
diff --git a/src/openassetio-core/include/openassetio/log/SeverityFilter.hpp b/src/openassetio-core/include/openassetio/log/SeverityFilter.hpp
index 55592afbf..b1a82655c 100644
--- a/src/openassetio-core/include/openassetio/log/SeverityFilter.hpp
+++ b/src/openassetio-core/include/openassetio/log/SeverityFilter.hpp
@@ -62,10 +62,35 @@ class OPENASSETIO_CORE_EXPORT SeverityFilter final : public LoggerInterface {
    */
   [[nodiscard]] LoggerInterface::Severity getSeverity() const;
 
+  /**
+   * Check if given severity will be logged.
+   *
+   * Uses the value of getSeverity() as well as querying the @ref
+   * upstreamLogger, and returns the most pessimistic answer.
+   *
+   * This logic is used in @ref log to determine which messages to
+   * filter out.
+   *
+   * @param severity Severity to check.
+   *
+   * @return Whether a log message at the given severity will be output.
+   */
+  [[nodiscard]] bool isSeverityLogged(Severity severity) const override;
   /**
    * @}
    */
 
+  /**
+   * Filter out messages based on severity before delegating to the
+   * @ref upstreamLogger.
+   *
+   * Whether a log is output or not obeys the result of @ref
+   * isSeverityLogged
+   *
+   * @param severity Severity level.
+   *
+   * @param message The message to be logged.
+   */
   void log(Severity severity, const Str& message) override;
 
  private:
diff --git a/src/openassetio-core/src/log/LoggerInterface.cpp b/src/openassetio-core/src/log/LoggerInterface.cpp
index bba8ab0e9..e8470c9e8 100644
--- a/src/openassetio-core/src/log/LoggerInterface.cpp
+++ b/src/openassetio-core/src/log/LoggerInterface.cpp
@@ -10,6 +10,10 @@ inline namespace OPENASSETIO_CORE_ABI_VERSION {
 namespace log {
 LoggerInterface::~LoggerInterface() = default;
 
+bool LoggerInterface::isSeverityLogged([[maybe_unused]] LoggerInterface::Severity severity) const {
+  return true;
+}
+
 void LoggerInterface::debugApi(const Str &message) { log(Severity::kDebugApi, message); }
 
 void LoggerInterface::debug(const Str &message) { log(Severity::kDebug, message); }
diff --git a/src/openassetio-core/src/log/SeverityFilter.cpp b/src/openassetio-core/src/log/SeverityFilter.cpp
index b4996bcda..d3ace0acc 100644
--- a/src/openassetio-core/src/log/SeverityFilter.cpp
+++ b/src/openassetio-core/src/log/SeverityFilter.cpp
@@ -34,7 +34,7 @@ SeverityFilter::SeverityFilter(LoggerInterfacePtr upstreamLogger)
 }
 
 void SeverityFilter::log(Severity severity, const Str& message) {
-  if (severity < minSeverity_) {
+  if (severity < minSeverity_ || !upstreamLogger_->isSeverityLogged(severity)) {
     return;
   }
   upstreamLogger_->log(severity, message);
@@ -45,6 +45,10 @@ void SeverityFilter::setSeverity(LoggerInterface::Severity severity) { minSeveri
 LoggerInterface::Severity SeverityFilter::getSeverity() const { return minSeverity_; }
 
 LoggerInterfacePtr SeverityFilter::upstreamLogger() const { return upstreamLogger_; }
+
+bool SeverityFilter::isSeverityLogged(LoggerInterface::Severity severity) const {
+  return severity >= minSeverity_ && upstreamLogger_->isSeverityLogged(severity);
+}
 }  // namespace log
 }  // namespace OPENASSETIO_CORE_ABI_VERSION
 }  // namespace openassetio
diff --git a/src/openassetio-python/cmodule/src/log/LoggerInterfaceBinding.cpp b/src/openassetio-python/cmodule/src/log/LoggerInterfaceBinding.cpp
index 96c53d134..8fccaa30e 100644
--- a/src/openassetio-python/cmodule/src/log/LoggerInterfaceBinding.cpp
+++ b/src/openassetio-python/cmodule/src/log/LoggerInterfaceBinding.cpp
@@ -21,6 +21,10 @@ struct PyLoggerInterface : LoggerInterface {
   void log(Severity severity, const Str& message) override {
     OPENASSETIO_PYBIND11_OVERRIDE_PURE(void, LoggerInterface, log, severity, message);
   }
+
+  [[nodiscard]] bool isSeverityLogged(Severity severity) const override {
+    OPENASSETIO_PYBIND11_OVERRIDE(bool, LoggerInterface, isSeverityLogged, severity);
+  }
 };
 }  // namespace log
 }  // namespace OPENASSETIO_CORE_ABI_VERSION
@@ -50,6 +54,8 @@ void registerLoggerInterface(const py::module& mod) {
   loggerInterface.def(py::init())
       .def("log", &LoggerInterface::log, py::arg("severity"), py::arg("message"),
            py::call_guard<py::gil_scoped_release>{})
+      .def("isSeverityLogged", &LoggerInterface::isSeverityLogged, py::arg("severity"),
+           py::call_guard<py::gil_scoped_release>{})
       .def("debugApi", &LoggerInterface::debugApi, py::arg("message"),
            py::call_guard<py::gil_scoped_release>{})
       .def("debug", &LoggerInterface::debug, py::arg("message"),


Test patch:
diff --git a/src/openassetio-python/tests/cmodule/gil/test_loggerinterface_gil.py b/src/openassetio-python/tests/cmodule/gil/test_loggerinterface_gil.py
index daeffad47..2089426bf 100644
--- a/src/openassetio-python/tests/cmodule/gil/test_loggerinterface_gil.py
+++ b/src/openassetio-python/tests/cmodule/gil/test_loggerinterface_gil.py
@@ -74,3 +74,6 @@ def test_progress(self, a_threaded_logger_interface):
 
     def test_warning(self, a_threaded_logger_interface):
         a_threaded_logger_interface.warning("")
+
+    def test_isSeverityLogged(self, a_threaded_logger_interface):
+        a_threaded_logger_interface.isSeverityLogged(LoggerInterface.Severity.kInfo)
diff --git a/src/openassetio-python/tests/conftest.py b/src/openassetio-python/tests/conftest.py
index 338a5d8d5..dddaad3e9 100644
--- a/src/openassetio-python/tests/conftest.py
+++ b/src/openassetio-python/tests/conftest.py
@@ -454,6 +454,9 @@ def __init__(self):
     def log(self, severity, message):
         self.mock.log(severity, message)
 
+    def isSeverityLogged(self, severity):
+        return self.mock.isSeverityLogged(severity)
+
 
 class MockEntityReferencePagerInterface(EntityReferencePagerInterface):
     """
diff --git a/src/openassetio-python/tests/package/log/test_loggerinterface.py b/src/openassetio-python/tests/package/log/test_loggerinterface.py
deleted file mode 100644
index 1ea35fb09..000000000
--- a/src/openassetio-python/tests/package/log/test_loggerinterface.py
+++ /dev/null
@@ -1,35 +0,0 @@
-#
-#   Copyright 2013-2023 The Foundry Visionmongers Ltd
-#
-#   Licensed under the Apache License, Version 2.0 (the "License");
-#   you may not use this file except in compliance with the License.
-#   You may obtain a copy of the License at
-#
-#       http://www.apache.org/licenses/LICENSE-2.0
-#
-#   Unless required by applicable law or agreed to in writing, software
-#   distributed under the License is distributed on an "AS IS" BASIS,
-#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-#   See the License for the specific language governing permissions and
-#   limitations under the License.
-#
-"""
-Tests that cover the openassetio.log.LoggerInterface class.
-"""
-
-# pylint: disable=invalid-name,redefined-outer-name
-# pylint: disable=missing-class-docstring,missing-function-docstring
-import pytest
-
-from openassetio.log import LoggerInterface
-
-
-class Test_LoggerInterface_log:
-    def test_is_pure_virtual(self, an_unimplemented_logger_interface):
-        with pytest.raises(RuntimeError, match="Tried to call pure virtual function"):
-            an_unimplemented_logger_interface.log(LoggerInterface.Severity.kInfo, "")
-
-
-@pytest.fixture
-def an_unimplemented_logger_interface():
-    return LoggerInterface()
diff --git a/src/openassetio-python/tests/package/test_log.py b/src/openassetio-python/tests/package/test_log.py
index 67b408bf2..ceaf4efbe 100644
--- a/src/openassetio-python/tests/package/test_log.py
+++ b/src/openassetio-python/tests/package/test_log.py
@@ -110,6 +110,18 @@ def test_when_shouldColorOutput_set_false_then_messages_are_not_colored(self, ca
         assert "\033[0m" not in output.err
 
 
+class Test_LoggerInterface_log:
+    def test_is_pure_virtual(self):
+        with pytest.raises(RuntimeError, match="Tried to call pure virtual function"):
+            lg.LoggerInterface().log(lg.LoggerInterface.Severity.kInfo, "")
+
+
+class Test_LoggerInterface_isSeverityLogged:
+    @pytest.mark.parametrize("severity", all_severities)
+    def test_when_not_overridden_then_always_returns_true(self, severity):
+        assert lg.LoggerInterface().isSeverityLogged(severity) is True
+
+
 class Test_LoggerInterface:
     def test_severity_names(self):
         assert (
@@ -277,6 +289,36 @@ def test_only_messages_of_equal_or_greater_severity_are_relayed(self, severity_f
                 else:
                     mock_logger.mock.log.assert_not_called()
 
+    @pytest.mark.parametrize("is_severity_logged", (True, False))
+    @pytest.mark.parametrize("filter_severity", all_severities)
+    @pytest.mark.parametrize("log_severity", all_severities)
+    def test_when_wrapped_and_wrapper_both_filter_by_severity_then_most_severe_used(
+        self, mock_logger, severity_filter, is_severity_logged, filter_severity, log_severity
+    ):
+        mock_logger.mock.isSeverityLogged.return_value = is_severity_logged
+        severity_filter.setSeverity(filter_severity)
+
+        severity_filter.log(log_severity, "a message")
+
+        assert mock_logger.mock.log.called == (
+            log_severity >= filter_severity and is_severity_logged
+        )
+
+
+class Test_SeverityFilter_isSeverityLogged:
+    @pytest.mark.parametrize("is_severity_logged", (True, False))
+    @pytest.mark.parametrize("filter_severity", all_severities)
+    @pytest.mark.parametrize("log_severity", all_severities)
+    def test_when_wrapped_and_wrapper_both_filter_by_severity_then_most_severe_used(
+        self, mock_logger, severity_filter, is_severity_logged, filter_severity, log_severity
+    ):
+        mock_logger.mock.isSeverityLogged.return_value = is_severity_logged
+        severity_filter.setSeverity(filter_severity)
+
+        assert severity_filter.isSeverityLogged(log_severity) == (
+            log_severity >= filter_severity and is_severity_logged
+        )
+
 
 class Test_SeverityFilter_upstreamLogger:
     def test_returns_the_constructor_supplied_logger(self, mock_logger):