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

Fix flush exception thrown when LoggerProvider not configured #3608

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

iurisilvio
Copy link
Contributor

@iurisilvio iurisilvio commented Jan 1, 2024

Description

I found #3423 fixing a similar issue and reused most of the idea. I preferred to write a test using the proper spec NoOpLoggerProvider instead of using the generic LogProvider and mocking the force_flush method.

I checked for other uses of logger_provider but didn't find any.

Fixes #3602

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit test

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

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

Happy new year! 🎉 🎆 🍰

@iurisilvio iurisilvio requested a review from a team January 1, 2024 09:36
Copy link

linux-foundation-easycla bot commented Jan 1, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: iurisilvio / name: Iuri de Silvio (1ddd505)
  • ✅ login: ocelotl / name: Diego Hurtado (41449c2, b219f01)

@iurisilvio iurisilvio force-pushed the fix-noop-logger-force-flush branch from d14f51f to 853bd75 Compare January 1, 2024 09:40
@iurisilvio iurisilvio force-pushed the fix-noop-logger-force-flush branch 2 times, most recently from 4abb135 to 2fe915e Compare January 9, 2024 22:48
@iurisilvio iurisilvio requested a review from pmcollins January 9, 2024 22:50
@iurisilvio iurisilvio requested a review from pmcollins January 9, 2024 23:55
@iurisilvio iurisilvio force-pushed the fix-noop-logger-force-flush branch from 0f04e76 to 3a3ea44 Compare January 19, 2024 13:29
@jeremydvoss jeremydvoss mentioned this pull request Feb 1, 2024
4 tasks
@ocelotl ocelotl force-pushed the fix-noop-logger-force-flush branch from 3a3ea44 to 1ddd505 Compare February 14, 2024 18:28
@ocelotl
Copy link
Contributor

ocelotl commented Feb 14, 2024

This test case passes but it is hiding an error:

py311-opentelemetry-sdk: commands[0] /home/tigre/github/ocelotl/opentelemetry-python/opentelemetry-sdk/tests> pytest -k test_log_flush_noop -s -v -rf
===================================================================================== test session starts ======================================================================================
platform linux -- Python 3.11.2, pytest-7.1.3, pluggy-1.4.0 -- /home/tigre/github/ocelotl/opentelemetry-python/.tox/py311-opentelemetry-sdk/bin/python
cachedir: .tox/py311-opentelemetry-sdk/.pytest_cache
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/tigre/github/ocelotl/opentelemetry-python, configfile: pyproject.toml
plugins: flaky-3.7.0, benchmark-4.0.0
collected 491 items / 490 deselected / 1 selected                                                                                                                                              

logs/test_handler.py::TestLoggingHandler::test_log_flush_noop PASSED

============================================================================== 1 passed, 490 deselected in 0.27s ===============================================================================
Exception ignored in atexit callback: <function shutdown at 0x7f519fff3240>
Traceback (most recent call last):
  File "/home/tigre/.pyenv/versions/3.11.2/lib/python3.11/logging/__init__.py", line 2192, in shutdown
    h.flush()
  File "/home/tigre/github/ocelotl/opentelemetry-python/.tox/py311-opentelemetry-sdk/lib/python3.11/site-packages/opentelemetry/sdk/_logs/_internal/__init__.py", line 559, in flush
    self._logger_provider.force_flush()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tigre/.pyenv/versions/3.11.2/lib/python3.11/unittest/mock.py", line 647, in __getattr__
    raise AttributeError("Mock object has no attribute %r" % name)
AttributeError: Mock object has no attribute 'force_flush'
  py311-opentelemetry-sdk: OK (10.91=setup[0.04]+cmd[1.41,8.91,0.55] seconds)
  congratulations :) (11.01 seconds)

@ocelotl ocelotl enabled auto-merge (squash) February 14, 2024 19:44
auto-merge was automatically disabled February 14, 2024 19:46

Pull Request is not mergeable

@ocelotl ocelotl merged commit fc9368f into open-telemetry:main Feb 14, 2024
232 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

NoOpLoggerProvider has no force_flush in exit callback
5 participants