chore: enforce kwarg logging #7207

wochinge · 2024-02-26T16:34:18Z

Related Issues

last part of Introduce structured logging #7027

Proposed Changes:

add a wrapped logger which enforces logging through kwargs

How did you test it?

added unit tests

Notes for the reviewer

Checklist

I have read the contributors guidelines and the code of conduct
I have updated the related issue with new insights and changes
I added unit tests and updated the docstrings
I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
I documented my code
I ran pre-commit hooks and fixed any issue

test/test_logging.py

haystack/components/rankers/meta_field.py

test/test_logging.py

haystack/logging.py

masci

Let's do this, I think the approach has several advantages:

it brings in consistency
it makes structure logging effective, not just an available feature
I like when Haystack is opinionated, and this is a great opportunity to do so

Approving the idea, code review to be done

wochinge · 2024-02-28T12:35:00Z

pyproject.toml

@@ -296,6 +296,7 @@ select = [
  "T10",    # flake8-debugger
  "W",      # pycodestyle
  "YTT",    # flake8-2020
+  "I"       # isort


I'm sorry for this as this causes a big diff but pylint complains about the import order for all logging imports (since the third-party import of logging became a first-party import from haystack) and I couldn't bother fixing them manually. In my opinion this should be enabled by default (and we should also run ruff in the CI!)

We tried but turned out we should rethink the whole lint/format workflow, see #5931

wochinge · 2024-02-28T12:35:26Z

haystack/__init__.py

@@ -1,9 +1,9 @@
+import haystack.logging


explanation for import order change here: https://github.com/deepset-ai/haystack/pull/7207/files#r1505884720

wochinge · 2024-02-28T13:21:59Z

test/test_logging.py

@@ -40,23 +41,6 @@ def test_skip_logging_configuration(
        # Nothing should be captured by capfd since structlog is not configured
        assert capfd.readouterr().err == ""

-    def test_skip_logging_if_structlog_not_installed(


this somehow breaks pytest itself. I tried to fix it but couldn't figure out what happened. Would be nice to test the import error but in my opinion also not super crucial (it's hard to test in the first place)

wochinge · 2024-02-28T15:47:19Z

haystack/logging.py

+    return wrapper
+
+
+def _patch_structlog_call_information(logger: logging.Logger) -> None:


I wholeheartedly apologize for this 😫

created an issue for this as well: hynek/structlog#602

coveralls · 2024-02-28T15:59:03Z

Pull Request Test Coverage Report for Build 8096409462

Details

0 of 0 changed or added relevant lines in 0 files are covered.
242 unchanged lines in 42 files lost coverage.
Overall coverage decreased (-0.2%) to 89.901%

Files with Coverage Reduction	New Missed Lines	%
components/preprocessors/document_cleaner.py	1	98.81%
components/routers/conditional_router.py	1	97.18%
components/websearch/searchapi.py	1	96.3%
components/writers/document_writer.py	1	97.37%
core/type_utils.py	1	97.56%
tracing/opentelemetry.py	1	97.14%
components/converters/azure.py	2	91.53%
components/converters/txt.py	2	90.0%
components/embedders/hugging_face_tei_document_embedder.py	2	95.16%
components/embedders/hugging_face_tei_text_embedder.py	2	92.68%

Totals
Change from base Build 8095219408:	-0.2%
Covered Lines:	5279
Relevant Lines:	5872

💛 - Coveralls

masci

Left one question but it looks very good to me! Now that I see in action I'm even more convinced this is the right approach, love it!

I might as well miss something but I swear I went through all the changed files 😅

masci · 2024-02-29T10:57:01Z

pyproject.toml

@@ -296,6 +296,7 @@ select = [
  "T10",    # flake8-debugger
  "W",      # pycodestyle
  "YTT",    # flake8-2020
+  "I"       # isort


We tried but turned out we should rethink the whole lint/format workflow, see #5931

masci · 2024-02-29T11:14:24Z

haystack/components/generators/openai.py

                "Increase the max_tokens parameter to allow for longer completions.",
-                message.meta["index"],
+                index=message.meta["index"],
+                finish_reason=message.meta["finish_reason"],


I can't see the {finish_reason} placeholder, is that missing?

great catch - this is actually an extra key-value which I found useful to be logged (so it's not interpolated in the message, just attached as key value)

github-actions bot added topic:tests topic:core type:documentation Improvements on the docs labels Feb 26, 2024

wochinge commented Feb 26, 2024

View reviewed changes

test/test_logging.py Show resolved Hide resolved

wochinge changed the title ~~Chore/enforce extra logging~~ chore: enforce extra logging Feb 26, 2024

wochinge force-pushed the chore/enforce-extra-logging branch from d2c7ae5 to aa80a38 Compare February 26, 2024 17:03

wochinge commented Feb 26, 2024

View reviewed changes

test/test_logging.py Outdated Show resolved Hide resolved

Base automatically changed from feat/structured-logging to main February 27, 2024 08:15

wochinge force-pushed the chore/enforce-extra-logging branch from aa80a38 to 3cca2e1 Compare February 27, 2024 08:17

github-actions bot added the 2.x Related to Haystack v2.0 label Feb 27, 2024

wochinge commented Feb 27, 2024

View reviewed changes

haystack/components/rankers/meta_field.py Show resolved Hide resolved

wochinge commented Feb 27, 2024

View reviewed changes

test/test_logging.py Show resolved Hide resolved

wochinge commented Feb 28, 2024

View reviewed changes

haystack/logging.py Show resolved Hide resolved

wochinge commented Feb 28, 2024

View reviewed changes

haystack/logging.py Show resolved Hide resolved

wochinge changed the title ~~chore: enforce extra logging~~ chore: enforce kwarg logging Feb 28, 2024

masci approved these changes Feb 28, 2024

View reviewed changes

wochinge added 4 commits February 28, 2024 11:56

chore: add logger which eases logging of extras

9146398

chore: start migrating to key value

dc984d8

fix: import fixes

05db3ce

tests: temporarily comment out breaking test

f598266

wochinge force-pushed the chore/enforce-extra-logging branch from 8269160 to f598266 Compare February 28, 2024 10:56

wochinge added 2 commits February 28, 2024 13:29

refactor: move to kwarg based logging

ff62ec7

style: fix import order

f148252

github-actions bot added the topic:build/distribution label Feb 28, 2024

wochinge commented Feb 28, 2024

View reviewed changes

chore: implement self-review comments

355bf50

wochinge force-pushed the chore/enforce-extra-logging branch from 9cdad45 to 355bf50 Compare February 28, 2024 12:54

wochinge added 2 commits February 28, 2024 14:17

test: drop failing test

afbe6da

chore: fix more import orders

7c89cfa

wochinge commented Feb 28, 2024

View reviewed changes

wochinge added 4 commits February 28, 2024 14:29

docs: add changelog

4ca1c69

tests: fix broken tests

c530238

chore: fix getting the frames

135d909

Merge branch 'main' into chore/enforce-extra-logging

d05f36c

wochinge commented Feb 28, 2024

View reviewed changes

wochinge added 2 commits February 28, 2024 16:48

chore: add comment

a39878b

chore: cleanup

da70fc1

wochinge requested a review from masci February 28, 2024 16:15

wochinge marked this pull request as ready for review February 28, 2024 16:15

wochinge requested review from a team as code owners February 28, 2024 16:15

wochinge requested review from dfokina and silvanocerza and removed request for a team February 28, 2024 16:15

chore: adapt remaining %s usages

6947304

masci approved these changes Feb 29, 2024

View reviewed changes

Merge branch 'main' into chore/enforce-extra-logging

29f5ce8

wochinge mentioned this pull request Feb 29, 2024

Sort imports with isort #5931

Closed

wochinge merged commit fe0ac5c into main Feb 29, 2024
23 of 24 checks passed

wochinge deleted the chore/enforce-extra-logging branch February 29, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: enforce kwarg logging #7207

chore: enforce kwarg logging #7207

wochinge commented Feb 26, 2024 •

edited

Loading

masci left a comment

wochinge Feb 28, 2024

masci Feb 29, 2024

wochinge Feb 28, 2024

wochinge Feb 28, 2024

wochinge Feb 28, 2024

wochinge Feb 28, 2024

coveralls commented Feb 28, 2024 •

edited

Loading

masci left a comment

masci Feb 29, 2024

masci Feb 29, 2024

wochinge Feb 29, 2024

		return wrapper


		def _patch_structlog_call_information(logger: logging.Logger) -> None:

chore: enforce kwarg logging #7207

chore: enforce kwarg logging #7207

Conversation

wochinge commented Feb 26, 2024 • edited Loading

Related Issues

Proposed Changes:

How did you test it?

Notes for the reviewer

Checklist

masci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 28, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8096409462

Details

💛 - Coveralls

masci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wochinge commented Feb 26, 2024 •

edited

Loading

coveralls commented Feb 28, 2024 •

edited

Loading