Skip to content

fix(opentelemetry-instrumentation-logging): Ensure that logging is pr…#2384

Closed
dhofstetter wants to merge 2 commits intoopen-telemetry:mainfrom
yiogmbh:fix/logging-instrumentation
Closed

fix(opentelemetry-instrumentation-logging): Ensure that logging is pr…#2384
dhofstetter wants to merge 2 commits intoopen-telemetry:mainfrom
yiogmbh:fix/logging-instrumentation

Conversation

@dhofstetter
Copy link
Copy Markdown
Contributor

@dhofstetter dhofstetter commented Mar 30, 2024 •
edited
Loading

…operly configured independently of the order of instrumentation (this helps especially in case of autoinstrumentation)

  • add method to perform proper logging initialisation
  • add unittests

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
I have changed the logging instrumentation in a way that ensures proper log setup no matter if the application (log) was already configured.
The changes are in a way s.t. we check before applying log level and format to check if basicConfig should be used. Otherwise we are going to modify the log level and handlers "manually".

I also changed the uninstrument method (which is mainly for the test_uninstrumented test. Because as soon as we changed the log format and we "uninstrument" the application, then the logs cannot be formatted anymore.

The changes are highly inspired by this open pull request. So props, to figure out a neat way to resolve this issue go to @rjduffner.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have written unit tests to prove that everything should work as expected.

To verify this with some application, you can locally install the repo and perform auto-instrumentation of an application. There it can be verified that the issues mentioned above are gone.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Mar 30, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from ocelotl March 30, 2024 21:49
@rjduffner
Copy link
Copy Markdown
Contributor

rjduffner commented Apr 2, 2024

Nice @dhofstetter. Thanks for taking this on.

Looks like my pr #1939 is fully implemented here. I can close #1939 when this is done.

@dhofstetter dhofstetter force-pushed the fix/logging-instrumentation branch 2 times, most recently from 5ebe8c8 to 02f5edb Compare April 25, 2024 16:27
@dhofstetter dhofstetter requested a review from a team April 25, 2024 16:27
@dhofstetter dhofstetter force-pushed the fix/logging-instrumentation branch 5 times, most recently from 3e45d57 to c7264c1 Compare April 28, 2024 15:20
xrmx
xrmx reviewed Apr 29, 2024
View reviewed changes
@dhofstetter dhofstetter force-pushed the fix/logging-instrumentation branch 2 times, most recently from 40d5db1 to fb3ceab Compare April 30, 2024 06:27
@dhofstetter dhofstetter requested a review from xrmx April 30, 2024 06:28
xrmx
xrmx reviewed Apr 30, 2024
View reviewed changes
@dhofstetter dhofstetter mentioned this pull request May 1, 2024
Closed
7 tasks
@dhofstetter dhofstetter force-pushed the fix/logging-instrumentation branch from fb3ceab to 0854421 Compare May 21, 2024 21:58
@dhofstetter
Copy link
Copy Markdown
Contributor Author

dhofstetter commented May 21, 2024

Changed according to suggestions and rebased to main

@dhofstetter
fix(opentelemetry-instrumentation-logging): Ensure that logging is pr…
c30fa40 
…operly configured independently of the order of instrumentation (this helps especially in case of autoinstrumentation)

- add method to perform proper logging initialisation
- add unittests
- extend CHANGELOG
@dhofstetter
changelog adjusted as proposed in review
51ccb36
@dhofstetter dhofstetter force-pushed the fix/logging-instrumentation branch from 0854421 to 51ccb36 Compare June 11, 2024 21:29
@dhofstetter
Copy link
Copy Markdown
Contributor Author

dhofstetter commented Jun 11, 2024

Rebased again to main.

@ocelotl Would you have your final look onto this pr?

@lzchen lzchen mentioned this pull request Jul 2, 2024
Open
lzchen
lzchen reviewed Jul 8, 2024
View reviewed changes
.../opentelemetry-instrumentation-logging/src/opentelemetry/instrumentation/logging/__init__.py
if root_logger.hasHandlers():
root_logger.setLevel(level)
for handler in root_logger.handlers:
handler.setFormatter(logging.Formatter(format))
Copy link
Copy Markdown
Contributor

@lzchen lzchen Jul 8, 2024

Choose a reason for hiding this comment

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

Shouldn't we be only reconfiguring the format for the OpenTelemetry LoggingHandler and not all of the user's handlers?

@ocelotl ocelotl removed their assignment Sep 3, 2024
@tdonovic tdonovic mentioned this pull request Jul 6, 2025
Open
@croziere
Copy link
Copy Markdown

croziere commented Feb 6, 2026

Hey @dhofstetter, this fix could greatly help us! What's the status of it? I'd be glad to help if needed

@MikeGoldsmith
Copy link
Copy Markdown
Member

MikeGoldsmith commented Feb 9, 2026

Same here, I'm willing to help out on this issue & potential fix.

MikeGoldsmith added a commit to honeycombio/opentelemetry-python-contrib that referenced this pull request Feb 13, 2026
@MikeGoldsmith
fix(logging): apply format/level when logging preconfigured
6c9b916 
basicConfig() no-op when handlers exist (autoinstrumentation, Gunicorn, Flask).
Fix: update existing handlers instead of only calling basicConfig.
Saves/restores state for clean uninstrumentation.

Supersedes open-telemetry#2384, fixes open-telemetry#2346
@MikeGoldsmith MikeGoldsmith mentioned this pull request Feb 13, 2026
Closed
10 tasks
@MikeGoldsmith
Copy link
Copy Markdown
Member

MikeGoldsmith commented Feb 13, 2026

Opened the following PR as this has been stale for 18 months:

@MikeGoldsmith
Copy link
Copy Markdown
Member

MikeGoldsmith commented Mar 3, 2026

This should be closed in favour of the following PR:

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 18, 2026

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

@github-actions github-actions bot added the Stale label Mar 18, 2026
@xrmx
Copy link
Copy Markdown
Contributor

xrmx commented Mar 18, 2026

We moved the code to the logging instrumentation so the changes should be done there.

@xrmx xrmx closed this Mar 18, 2026
@github-project-automation github-project-automation bot moved this from Ready for review to Done in Python PR digest Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xrmx xrmx xrmx left review comments

@pmcollins pmcollins pmcollins left review comments

@lzchen lzchen lzchen left review comments

@ocelotl ocelotl Awaiting requested review from ocelotl

+1 more reviewer

@ilkerhat ilkerhat ilkerhat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Stale

Projects

Python PR digest
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OTEL_PYTHON_LOG_CORRELATION is mentioned in doc, but does not seem to work.

9 participants

@dhofstetter @rjduffner @croziere @MikeGoldsmith @xrmx @pmcollins @lzchen @ilkerhat @ocelotl