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

sdk: decouple logs sdk setup from OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED #4340

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

- Always setup logs sdk, OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED only controls python `logging` module handler setup
([#4340](https://github.com/open-telemetry/opentelemetry-python/pull/4340))
- Fix crash exporting a log record with None body
([#4276](https://github.com/open-telemetry/opentelemetry-python/pull/4276))
- Fix metrics export with exemplar and no context and filtering observable instruments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ def _init_metrics(
def _init_logging(
exporters: Dict[str, Type[LogExporter]],
resource: Resource = None,
setup_logging_handler: bool = True,
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 True to keep compatibility with old callers. I know it's a private function but still.

):
provider = LoggerProvider(resource=resource)
set_logger_provider(provider)
Expand All @@ -246,13 +247,15 @@ def _init_logging(
BatchLogRecordProcessor(exporter_class(**exporter_args))
)

handler = LoggingHandler(level=logging.NOTSET, logger_provider=provider)

logging.getLogger().addHandler(handler)

event_logger_provider = EventLoggerProvider(logger_provider=provider)
set_event_logger_provider(event_logger_provider)

if setup_logging_handler:
handler = LoggingHandler(
level=logging.NOTSET, logger_provider=provider
)
logging.getLogger().addHandler(handler)


def _import_exporters(
trace_exporter_names: Sequence[str],
Expand Down Expand Up @@ -364,7 +367,7 @@ def _initialize_components(
sampler: Optional[Sampler] = None,
resource_attributes: Optional[Attributes] = None,
id_generator: IdGenerator = None,
logging_enabled: Optional[bool] = None,
setup_logging_handler: Optional[bool] = None,
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 will change behavior for custom distro implementors and so I've renamed the param name

Copy link
Member

Choose a reason for hiding this comment

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

Much clearer too 👍

):
if trace_exporter_names is None:
trace_exporter_names = []
Expand Down Expand Up @@ -401,17 +404,16 @@ def _initialize_components(
resource=resource,
)
_init_metrics(metric_exporters, resource)
if logging_enabled is None:
logging_enabled = (
if setup_logging_handler is None:
setup_logging_handler = (
os.getenv(
_OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED, "false"
)
.strip()
.lower()
== "true"
)
if logging_enabled:
_init_logging(log_exporters, resource)
_init_logging(log_exporters, resource, setup_logging_handler)


class _BaseConfigurator(ABC):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@
.. envvar:: OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED

The :envvar:`OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED` environment variable allows users to
enable/disable the logging SDK auto instrumentation.
enable/disable the auto instrumentation for the python logging module.
Default: False

Note: Logs SDK and its related settings are experimental.
Expand Down
35 changes: 31 additions & 4 deletions opentelemetry-sdk/tests/test_configurator.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from logging import WARNING, getLogger
from os import environ
from typing import Dict, Iterable, Optional, Sequence
from unittest import TestCase
from unittest import TestCase, mock
from unittest.mock import Mock, patch

from pytest import raises
Expand Down Expand Up @@ -663,6 +663,32 @@ def test_logging_init_exporter(self):
getLogger(__name__).error("hello")
self.assertTrue(provider.processor.exporter.export_called)

@patch.dict(
environ,
{"OTEL_RESOURCE_ATTRIBUTES": "service.name=otlp-service"},
)
def test_logging_init_exporter_without_handler_setup(self):
resource = Resource.create({})
_init_logging(
{"otlp": DummyOTLPLogExporter},
resource=resource,
setup_logging_handler=False,
)
self.assertEqual(self.set_provider_mock.call_count, 1)
provider = self.set_provider_mock.call_args[0][0]
self.assertIsInstance(provider, DummyLoggerProvider)
self.assertIsInstance(provider.resource, Resource)
self.assertEqual(
provider.resource.attributes.get("service.name"),
"otlp-service",
)
self.assertIsInstance(provider.processor, DummyLogRecordProcessor)
self.assertIsInstance(
provider.processor.exporter, DummyOTLPLogExporter
)
getLogger(__name__).error("hello")
self.assertFalse(provider.processor.exporter.export_called)

@patch.dict(
environ,
{"OTEL_RESOURCE_ATTRIBUTES": "service.name=otlp-service"},
Expand All @@ -671,8 +697,8 @@ def test_logging_init_exporter(self):
@patch("opentelemetry.sdk._configuration._init_logging")
def test_logging_init_disable_default(self, logging_mock, tracing_mock):
_initialize_components(auto_instrumentation_version="auto-version")
self.assertEqual(logging_mock.call_count, 0)
self.assertEqual(tracing_mock.call_count, 1)
logging_mock.assert_called_once_with(mock.ANY, mock.ANY, False)

@patch.dict(
environ,
Expand All @@ -686,7 +712,7 @@ def test_logging_init_disable_default(self, logging_mock, tracing_mock):
def test_logging_init_enable_env(self, logging_mock, tracing_mock):
with self.assertLogs(level=WARNING):
_initialize_components(auto_instrumentation_version="auto-version")
self.assertEqual(logging_mock.call_count, 1)
logging_mock.assert_called_once_with(mock.ANY, mock.ANY, True)
self.assertEqual(tracing_mock.call_count, 1)

@patch.dict(
Expand Down Expand Up @@ -768,7 +794,7 @@ def test_initialize_components_kwargs(
"custom.key.2": "pass-in-value-2",
},
"id_generator": "TEST_GENERATOR",
"logging_enabled": True,
"setup_logging_handler": True,
}
_initialize_components(**kwargs)

Expand Down Expand Up @@ -810,6 +836,7 @@ def test_initialize_components_kwargs(
logging_mock.assert_called_once_with(
"TEST_LOG_EXPORTERS_DICT",
"TEST_RESOURCE",
True,
)


Expand Down
Loading