From a49fed8a2ea074b4e4c5f64697a19c8b2b4e4e0b Mon Sep 17 00:00:00 2001 From: GanymedeNil Date: Mon, 12 Jun 2023 23:29:19 +0800 Subject: [PATCH 1/7] Update the body type in the log --- .../src/opentelemetry/sdk/_logs/_internal/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py index 7410138067c..12497fc7c08 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py @@ -483,7 +483,7 @@ def _translate(self, record: logging.LogRecord) -> LogRecord: trace_flags=span_context.trace_flags, severity_text=record.levelname, severity_number=severity_number, - body=record.getMessage(), + body=record.msg, resource=self._logger.resource, attributes=attributes, ) From b4f44a2c4362495af0ca5c581dac456d6426cd6a Mon Sep 17 00:00:00 2001 From: GanymedeNil Date: Mon, 12 Jun 2023 23:56:43 +0800 Subject: [PATCH 2/7] Updated changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ce6fc42516..637ac8325ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - Use BoundedAttributes instead of raw dict to extract attributes from LogRecord and Support dropped_attributes_count in LogRecord ([#3310](https://github.com/open-telemetry/opentelemetry-python/pull/3310)) +- Update the body type in the log + ([$3343](https://github.com/open-telemetry/opentelemetry-python/pull/3343)) ## Version 1.18.0/0.39b0 (2023-05-04) - Select histogram aggregation with an environment variable From 4595953fc6c956a0d62d75e3d435958c8c4d565b Mon Sep 17 00:00:00 2001 From: GanymedeNil Date: Thu, 15 Jun 2023 19:37:33 +0800 Subject: [PATCH 3/7] Fix formatted log --- .../sdk/_logs/_internal/__init__.py | 102 +++++++++--------- 1 file changed, 53 insertions(+), 49 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py index 12497fc7c08..71bda5474c7 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py @@ -88,9 +88,9 @@ class LogLimits: UNSET = -1 def __init__( - self, - max_attributes: Optional[int] = None, - max_attribute_length: Optional[int] = None, + self, + max_attributes: Optional[int] = None, + max_attribute_length: Optional[int] = None, ): # attribute count @@ -114,7 +114,7 @@ def __repr__(self): @classmethod def _from_env_if_absent( - cls, value: Optional[int], env_var: str, default: Optional[int] = None + cls, value: Optional[int], env_var: str, default: Optional[int] = None ) -> Optional[int]: if value == cls.UNSET: return None @@ -156,18 +156,18 @@ class LogRecord(APILogRecord): """ def __init__( - self, - timestamp: Optional[int] = None, - observed_timestamp: Optional[int] = None, - trace_id: Optional[int] = None, - span_id: Optional[int] = None, - trace_flags: Optional[TraceFlags] = None, - severity_text: Optional[str] = None, - severity_number: Optional[SeverityNumber] = None, - body: Optional[Any] = None, - resource: Optional[Resource] = None, - attributes: Optional[Attributes] = None, - limits: Optional[LogLimits] = _UnsetLogLimits, + self, + timestamp: Optional[int] = None, + observed_timestamp: Optional[int] = None, + trace_id: Optional[int] = None, + span_id: Optional[int] = None, + trace_flags: Optional[TraceFlags] = None, + severity_text: Optional[str] = None, + severity_number: Optional[SeverityNumber] = None, + body: Optional[Any] = None, + resource: Optional[Resource] = None, + attributes: Optional[Attributes] = None, + limits: Optional[LogLimits] = _UnsetLogLimits, ): super().__init__( **{ @@ -229,9 +229,9 @@ class LogData: """Readable LogRecord data plus associated InstrumentationLibrary.""" def __init__( - self, - log_record: LogRecord, - instrumentation_scope: InstrumentationScope, + self, + log_record: LogRecord, + instrumentation_scope: InstrumentationScope, ): self.log_record = log_record self.instrumentation_scope = instrumentation_scope @@ -284,7 +284,7 @@ def __init__(self): self._lock = threading.Lock() def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ) -> None: """Adds a Logprocessor to the list of log processors handled by this instance""" with self._lock: @@ -346,16 +346,16 @@ def __init__(self, max_workers: int = 2): ) def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ): with self._lock: self._log_record_processors += (log_record_processor,) def _submit_and_wait( - self, - func: Callable[[LogRecordProcessor], Callable[..., None]], - *args: Any, - **kwargs: Any, + self, + func: Callable[[LogRecordProcessor], Callable[..., None]], + *args: Any, + **kwargs: Any, ): futures = [] for lp in self._log_record_processors: @@ -437,9 +437,9 @@ class LoggingHandler(logging.Handler): """ def __init__( - self, - level=logging.NOTSET, - logger_provider=None, + self, + level=logging.NOTSET, + logger_provider=None, ) -> None: super().__init__(level=level) self._logger_provider = logger_provider or get_logger_provider() @@ -476,6 +476,10 @@ def _translate(self, record: logging.LogRecord) -> LogRecord: span_context = get_current_span().get_span_context() attributes = self._get_attributes(record) severity_number = std_to_otel(record.levelno) + if isinstance(record.msg, str) and record.args: + body = record.msg % record.args + else: + body = record.msg return LogRecord( timestamp=timestamp, trace_id=span_context.trace_id, @@ -483,7 +487,7 @@ def _translate(self, record: logging.LogRecord) -> LogRecord: trace_flags=span_context.trace_flags, severity_text=record.levelname, severity_number=severity_number, - body=record.msg, + body=body, resource=self._logger.resource, attributes=attributes, ) @@ -505,13 +509,13 @@ def flush(self) -> None: class Logger(APILogger): def __init__( - self, - resource: Resource, - multi_log_record_processor: Union[ - SynchronousMultiLogRecordProcessor, - ConcurrentMultiLogRecordProcessor, - ], - instrumentation_scope: InstrumentationScope, + self, + resource: Resource, + multi_log_record_processor: Union[ + SynchronousMultiLogRecordProcessor, + ConcurrentMultiLogRecordProcessor, + ], + instrumentation_scope: InstrumentationScope, ): super().__init__( instrumentation_scope.name, @@ -536,17 +540,17 @@ def emit(self, record: LogRecord): class LoggerProvider(APILoggerProvider): def __init__( - self, - resource: Resource = Resource.create(), - shutdown_on_exit: bool = True, - multi_log_record_processor: Union[ - SynchronousMultiLogRecordProcessor, - ConcurrentMultiLogRecordProcessor, - ] = None, + self, + resource: Resource = Resource.create(), + shutdown_on_exit: bool = True, + multi_log_record_processor: Union[ + SynchronousMultiLogRecordProcessor, + ConcurrentMultiLogRecordProcessor, + ] = None, ): self._resource = resource self._multi_log_record_processor = ( - multi_log_record_processor or SynchronousMultiLogRecordProcessor() + multi_log_record_processor or SynchronousMultiLogRecordProcessor() ) self._at_exit_handler = None if shutdown_on_exit: @@ -557,10 +561,10 @@ def resource(self): return self._resource def get_logger( - self, - name: str, - version: Optional[str] = None, - schema_url: Optional[str] = None, + self, + name: str, + version: Optional[str] = None, + schema_url: Optional[str] = None, ) -> Logger: return Logger( self._resource, @@ -573,7 +577,7 @@ def get_logger( ) def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ): """Registers a new :class:`LogRecordProcessor` for this `LoggerProvider` instance. From eef7ea1ee2a2466619ddd86ac3c953423e397834 Mon Sep 17 00:00:00 2001 From: GanymedeNil Date: Thu, 15 Jun 2023 20:01:30 +0800 Subject: [PATCH 4/7] add different msg types tests --- opentelemetry-sdk/tests/logs/test_export.py | 34 +++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/opentelemetry-sdk/tests/logs/test_export.py b/opentelemetry-sdk/tests/logs/test_export.py index aa68b096241..14924a30cca 100644 --- a/opentelemetry-sdk/tests/logs/test_export.py +++ b/opentelemetry-sdk/tests/logs/test_export.py @@ -167,6 +167,40 @@ def test_simple_log_record_processor_shutdown(self): finished_logs = exporter.get_finished_logs() self.assertEqual(len(finished_logs), 0) + def test_simple_log_record_processor_different_msg_types(self): + exporter = InMemoryLogExporter() + log_record_processor = BatchLogRecordProcessor(exporter) + + provider = LoggerProvider() + provider.add_log_record_processor(log_record_processor) + + logger = logging.getLogger("different_msg_types") + logger.addHandler(LoggingHandler(logger_provider=provider)) + + logger.warning("warning message: %s", "possible upcoming heatwave") + logger.error("Very high rise in temperatures across the globe") + logger.critical("Temperature hits high 420 C in Hyderabad") + logger.warning(["list", "of", "strings"]) + logger.error({"key": "value"}) + log_record_processor.shutdown() + + finished_logs = exporter.get_finished_logs() + expected = [ + ("warning message: possible upcoming heatwave", "WARNING"), + ("Very high rise in temperatures across the globe", "ERROR"), + ( + "Temperature hits high 420 C in Hyderabad", + "CRITICAL", + ), + (["list", "of", "strings"], "WARNING"), + ({"key": "value"}, "ERROR") + ] + emitted = [ + (item.log_record.body, item.log_record.severity_text) + for item in finished_logs + ] + self.assertEqual(expected, emitted) + class TestBatchLogRecordProcessor(ConcurrencyTestBase): def test_emit_call_log_record(self): From f72d854e7285d6801b03baa722cfae34e9655a2d Mon Sep 17 00:00:00 2001 From: GanymedeNil Date: Thu, 29 Jun 2023 00:37:33 +0800 Subject: [PATCH 5/7] restore format --- .../sdk/_logs/_internal/__init__.py | 96 +++++++++---------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py index 71bda5474c7..cbaaace8e35 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py @@ -88,9 +88,9 @@ class LogLimits: UNSET = -1 def __init__( - self, - max_attributes: Optional[int] = None, - max_attribute_length: Optional[int] = None, + self, + max_attributes: Optional[int] = None, + max_attribute_length: Optional[int] = None, ): # attribute count @@ -114,7 +114,7 @@ def __repr__(self): @classmethod def _from_env_if_absent( - cls, value: Optional[int], env_var: str, default: Optional[int] = None + cls, value: Optional[int], env_var: str, default: Optional[int] = None ) -> Optional[int]: if value == cls.UNSET: return None @@ -156,18 +156,18 @@ class LogRecord(APILogRecord): """ def __init__( - self, - timestamp: Optional[int] = None, - observed_timestamp: Optional[int] = None, - trace_id: Optional[int] = None, - span_id: Optional[int] = None, - trace_flags: Optional[TraceFlags] = None, - severity_text: Optional[str] = None, - severity_number: Optional[SeverityNumber] = None, - body: Optional[Any] = None, - resource: Optional[Resource] = None, - attributes: Optional[Attributes] = None, - limits: Optional[LogLimits] = _UnsetLogLimits, + self, + timestamp: Optional[int] = None, + observed_timestamp: Optional[int] = None, + trace_id: Optional[int] = None, + span_id: Optional[int] = None, + trace_flags: Optional[TraceFlags] = None, + severity_text: Optional[str] = None, + severity_number: Optional[SeverityNumber] = None, + body: Optional[Any] = None, + resource: Optional[Resource] = None, + attributes: Optional[Attributes] = None, + limits: Optional[LogLimits] = _UnsetLogLimits, ): super().__init__( **{ @@ -229,9 +229,9 @@ class LogData: """Readable LogRecord data plus associated InstrumentationLibrary.""" def __init__( - self, - log_record: LogRecord, - instrumentation_scope: InstrumentationScope, + self, + log_record: LogRecord, + instrumentation_scope: InstrumentationScope, ): self.log_record = log_record self.instrumentation_scope = instrumentation_scope @@ -284,7 +284,7 @@ def __init__(self): self._lock = threading.Lock() def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ) -> None: """Adds a Logprocessor to the list of log processors handled by this instance""" with self._lock: @@ -346,16 +346,16 @@ def __init__(self, max_workers: int = 2): ) def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ): with self._lock: self._log_record_processors += (log_record_processor,) def _submit_and_wait( - self, - func: Callable[[LogRecordProcessor], Callable[..., None]], - *args: Any, - **kwargs: Any, + self, + func: Callable[[LogRecordProcessor], Callable[..., None]], + *args: Any, + **kwargs: Any, ): futures = [] for lp in self._log_record_processors: @@ -437,9 +437,9 @@ class LoggingHandler(logging.Handler): """ def __init__( - self, - level=logging.NOTSET, - logger_provider=None, + self, + level=logging.NOTSET, + logger_provider=None, ) -> None: super().__init__(level=level) self._logger_provider = logger_provider or get_logger_provider() @@ -509,13 +509,13 @@ def flush(self) -> None: class Logger(APILogger): def __init__( - self, - resource: Resource, - multi_log_record_processor: Union[ - SynchronousMultiLogRecordProcessor, - ConcurrentMultiLogRecordProcessor, - ], - instrumentation_scope: InstrumentationScope, + self, + resource: Resource, + multi_log_record_processor: Union[ + SynchronousMultiLogRecordProcessor, + ConcurrentMultiLogRecordProcessor, + ], + instrumentation_scope: InstrumentationScope, ): super().__init__( instrumentation_scope.name, @@ -540,17 +540,17 @@ def emit(self, record: LogRecord): class LoggerProvider(APILoggerProvider): def __init__( - self, - resource: Resource = Resource.create(), - shutdown_on_exit: bool = True, - multi_log_record_processor: Union[ - SynchronousMultiLogRecordProcessor, - ConcurrentMultiLogRecordProcessor, - ] = None, + self, + resource: Resource = Resource.create(), + shutdown_on_exit: bool = True, + multi_log_record_processor: Union[ + SynchronousMultiLogRecordProcessor, + ConcurrentMultiLogRecordProcessor, + ] = None, ): self._resource = resource self._multi_log_record_processor = ( - multi_log_record_processor or SynchronousMultiLogRecordProcessor() + multi_log_record_processor or SynchronousMultiLogRecordProcessor() ) self._at_exit_handler = None if shutdown_on_exit: @@ -561,10 +561,10 @@ def resource(self): return self._resource def get_logger( - self, - name: str, - version: Optional[str] = None, - schema_url: Optional[str] = None, + self, + name: str, + version: Optional[str] = None, + schema_url: Optional[str] = None, ) -> Logger: return Logger( self._resource, @@ -577,7 +577,7 @@ def get_logger( ) def add_log_record_processor( - self, log_record_processor: LogRecordProcessor + self, log_record_processor: LogRecordProcessor ): """Registers a new :class:`LogRecordProcessor` for this `LoggerProvider` instance. From 03ef5dc7056bb5e06b0c241588a11d6d8c427aa9 Mon Sep 17 00:00:00 2001 From: Shalev Roda <65566801+shalevr@users.noreply.github.com> Date: Tue, 4 Jul 2023 16:11:03 +0300 Subject: [PATCH 6/7] Update CHANGELOG.md --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3982ffde02c..a57eca09022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased -- Use BoundedAttributes instead of raw dict to extract attributes from LogRecord and Support dropped_attributes_count in LogRecord ([#3310](https://github.com/open-telemetry/opentelemetry-python/pull/3310)) - Update the body type in the log ([$3343](https://github.com/open-telemetry/opentelemetry-python/pull/3343)) - Add max_scale option to Exponential Bucket Histogram Aggregation From c5ec6b9878e61684f2ad86122a2f4b01479a91f6 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 12 Jul 2023 18:20:33 +0200 Subject: [PATCH 7/7] Add comment from the PR --- .../sdk/_logs/_internal/__init__.py | 39 ++++++++++++++++++- opentelemetry-sdk/tests/logs/test_export.py | 2 +- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py index 3c19f46a9d5..eb5f50e9e0f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py @@ -21,7 +21,7 @@ import traceback from os import environ from time import time_ns -from typing import Any, Callable, Optional, Tuple, Union +from typing import Any, Callable, Optional, Tuple, Union # noqa from opentelemetry._logs import Logger as APILogger from opentelemetry._logs import LoggerProvider as APILoggerProvider @@ -476,6 +476,43 @@ def _translate(self, record: logging.LogRecord) -> LogRecord: timestamp = int(record.created * 1e9) span_context = get_current_span().get_span_context() attributes = self._get_attributes(record) + # This comment is taken from GanyedeNil's PR #3343, I have redacted it + # slightly for clarity: + # According to the definition of the Body field type in the + # OTel 1.22.0 Logs Data Model article, the Body field should be of + # type 'any' and should not use the str method to directly translate + # the msg. This is because str only converts non-text types into a + # human-readable form, rather than a standard format, which leads to + # the need for additional operations when collected through a log + # collector. + # Considering that he Body field should be of type 'any' and should not + # use the str method but record.msg is also a string type, then the + # difference is just the self.args formatting? + # The primary consideration depends on the ultimate purpose of the log. + # Converting the default log directly into a string is acceptable as it + # will be required to be presented in a more readable format. However, + # this approach might not be as "standard" when hoping to aggregate + # logs and perform subsequent data analysis. In the context of log + # extraction, it would be more appropriate for the msg to be + # converted into JSON format or remain unchanged, as it will eventually + # be transformed into JSON. If the final output JSON data contains a + # structure that appears similar to JSON but is not, it may confuse + # users. This is particularly true for operation and maintenance + # personnel who need to deal with log data in various languages. + # Where is the JSON converting occur? and what about when the msg + # represents something else but JSON, the expected behavior change? + # For the ConsoleLogExporter, it performs the to_json operation in + # opentelemetry.sdk._logs._internal.export.ConsoleLogExporter.__init__, + # so it can handle any type of input without problems. As for the + # OTLPLogExporter, it also handles any type of input encoding in + # _encode_log located in + # opentelemetry.exporter.otlp.proto.common._internal._log_encoder. + # Therefore, no extra operation is needed to support this change. + # The only thing to consider is the users who have already been using + # this SDK. If they upgrade the SDK after this change, they will need + # to readjust their logging collection rules to adapt to the latest + # output format. Therefore, this change is considered a breaking + # change and needs to be upgraded at an appropriate time. severity_number = std_to_otel(record.levelno) if isinstance(record.msg, str) and record.args: body = record.msg % record.args diff --git a/opentelemetry-sdk/tests/logs/test_export.py b/opentelemetry-sdk/tests/logs/test_export.py index c755230551b..2828504eaa9 100644 --- a/opentelemetry-sdk/tests/logs/test_export.py +++ b/opentelemetry-sdk/tests/logs/test_export.py @@ -199,7 +199,7 @@ def test_simple_log_record_processor_different_msg_types(self): "CRITICAL", ), (["list", "of", "strings"], "WARNING"), - ({"key": "value"}, "ERROR") + ({"key": "value"}, "ERROR"), ] emitted = [ (item.log_record.body, item.log_record.severity_text)