Add initial logging library SDK implementation #1882

srikanthccv · 2021-05-30T07:01:12Z

Description

Unlike tracing and metrics, logs specification doesn't define new logging API. Many existing logging libraries have some sort of extension mechanism that allows to customize how log records are encoded and delivered to their destinations. The OpenTelemetry Logging Library SDK is intended to be used by such extensions to emit logs in OpenTelemetry formats. In Python this can be achieved with logging.Handlers and Python standard library provides many handlers out of the box.

Our LogEmitter implements the base Handler interface defined in logging module and emits the log events in OpenTelemetry manner. The remaining parts of logging SDK resembles the tracing SDK to ensure consistency and less cognitive overhead. End users can simply plug-in the OTEL handler and get their logs exported to preferred backend in OTLP format. Small snippet to show how this can be done.

provider = get_log_emitter_provider()
exporter = OTLPLogExporter(...)
provider.add_log_processor(SimpleLogProcessor(exporter))
emitter = provider.get_log_emitter(__name__, "0.1")

logging.getLogger('root').addHandler(emitter)

This PR adds the functional logging sdk implementation without an exporters (I will send a follow up PR which adds support for OTLP exporters with more documentation and examples). We believe there won't be significantly drastic changes to SDK specification and providing a pre-release/experimental package helps us gather early feedback which will be helpful for both this SIG and logs SIG.

Type of change

New feature (non-breaking change which adds functionality)
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

tox -e test-core-sdk
tox -e docs
tox -e lint

Does This PR Require a Contrib Repo Change?

No.

Checklist:

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

srikanthccv · 2021-06-01T02:48:26Z

@open-telemetry/python-approvers ^

owais · 2021-06-01T09:32:41Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+_logger = logging.getLogger(__name__)
+
+
+class OTELLogRecord:


Is this prefixed with OTEL to avoid confusion with logging.LogRecord? Nothing else in this package has the OTEL prefix so this feels a bit inconsistent. May be we can live with LogRecord and let users figure out which one they're using. Somewhat similar to api/sdk objects with same name.

Admittedly, LogRecord.from_log_record(log_record) does feel a bit weird though but perhaps it won't be as bad if users import as aliases like we do with trace_api. We could also rename the method to something like from_std_log_record(record: logging.LogRecord) to make things a bit more obvious.

Yes, it was an attempt to avoid the confusion. On a second thought, I agree letting users to figure it out is fine.

Yes, prefixing classes with OTEL is redundant, Python imports are namespaced so it is not needed.

Also, prefixing every class with Log feels redundant as well.

ocelotl · 2021-06-02T00:38:12Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+_logger = logging.getLogger(__name__)
+
+
+class OTELLogRecord:


Yes, prefixing classes with OTEL is redundant, Python imports are namespaced so it is not needed.

ocelotl · 2021-06-02T00:41:36Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+        self.attributes = attributes
+
+    @classmethod
+    def from_log_record(


We are instantiating a new LogRecord here. I suggest we instead pass an optional record attribute to __init__ and use it to do what is being done here.

I prefer keeping it this way. This iteration of logging SDK only targeting the logs generated using standard logging library but eventually we will have to support the other popular third party logging libraries as well if they have strong user base. Then we would just introduce another from_popularlib_log_record.

If that's the case, this seems like a job for entry points instead of having hardcoded functions with the names of third party logging libraries.

I am not sure I understand what you mean by job for entry points. What is wrong with having multiple alternative constructors? This kind of takes an inspiration from patterns followed in stdlib datetime.fromtimestamp, datetime.fromordinal, and datetime.fromisocalendar etc.

@ocelotl How does the entry point look like when there are more the one logging lib involved?

Hm, I don't understand your question... I'm suggesting following the same approach we are following for instrumentations, you can use that as an example for what I am suggesting here. 🤷

I am really sorry but I can't visualise this and having difficulty in seeing the big picture. Can you share some example?

I was thinking about something like this:

Define an entry point for this purpose, let's say: opentelemetry_log_translator.

Define an abstract class that includes an abstract translate_log method` that should do the translation when implemented and an abstract property that returns the log record class when implemented.

A plugin is defined for every third party library, every plugin implements the abstract class defined before, every plugin has an opentelemetry_log_translator entry point that points to this class.

The SDK provides a function that receives a log record, looks for the translators in the opentelemetry_log_translator entry point, and if it finds one that matches the type of log record (by calling the property) then it calls the corresponding translation method and returns its result.

Thanks for the explanation. Yes, it makes sense to not to include the translate logic for each lib in LogRecord class.
Same question with more clarity, do we need an entry point for this?

Each plugin will grab a LogEmitter (analogous to Tracer in instrumentations) from provider and every time log event triggered then plugin translates it's event record to OTLP format and call emitter.emit. So to summarise what I am trying to say.

There will be a plugin for each lib which is responsible for doing all it takes to send an event from logging lib to Emitter.

This plugin will do the translation work and other necessary implementation detail to make it pluggable into original logging lib (example a stdlib plugin which subclasses Handler).

Enabling a plugin means hooking it to original logging lib which receives all the log event generated by it.

Yeah, I think that sounds like a good plan 👍

ocelotl · 2021-06-02T00:50:32Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+
+    def __eq__(self, other: object) -> bool:
+        if not isinstance(other, OTELLogRecord):
+            return NotImplemented


Suggested change

return NotImplemented

return False

I understand that the documentation of __eq__ mentions this:

A rich comparison method may return the singleton NotImplemented if it does not implement the operation for a given pair of arguments. By convention, False and True are returned for a successful comparison.

Nevertheless, I'm still not sure that we should limit this method to objects of the LogRecord class, since it is usually possible to make comparisons between different objects:

>>> 1 == "1" False >>> True == None False >>> 1.2 == False False

Why do you think we should not limit this to just LogRecord? I don't see a case where we would want to accept different object type and yet be able to decide if they are equal?

Why do you think we should not limit this to just LogRecord? I don't see a case where we would want to accept different object type and yet be able to decide if they are equal?

I see the opposite case, where we compare for equality a LogRecord object and a non-LogRecord object and we decide they are not equal. It makes sense to return False in that situation.

When does it make sense to compare LogRecord object with Non LogRecord object? What would the comparison look like and why would we want to do it?

ocelotl · 2021-06-02T01:33:15Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+
+
+class LogData:
+    """Readable LogRecord data plus associated Resource and InstrumentationLibrary."""


There is no resource in the code of this class 🤔

I need some clarification on this from spec. We have resource in LogRecord data model so I don't know why spec says we should have one here as well.

ocelotl · 2021-06-02T01:42:23Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+_logger = logging.getLogger(__name__)
+
+
+class OTELLogRecord:


Also, prefixing every class with Log feels redundant as well.

ocelotl · 2021-06-02T01:46:42Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+        return True
+
+
+class LogEmitter(logging.Handler):


I feel like this should be named Handler.

I thought of naming this OTLPHandler along the lines of default stdlib SMTPHandler but then just named it after what spec says . No strong preference. Others can chime in share their opinion.

ocelotl · 2021-06-02T02:42:04Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/severity.py

+    11: SeverityNumber.DEBUG2,
+    12: SeverityNumber.DEBUG3,
+    13: SeverityNumber.DEBUG4,
+    14: SeverityNumber.DEBUG4,
+    15: SeverityNumber.DEBUG4,
+    16: SeverityNumber.DEBUG4,
+    17: SeverityNumber.DEBUG4,
+    18: SeverityNumber.DEBUG4,
+    19: SeverityNumber.DEBUG4,


I don't think we should map this way. Considering these logging levels for Python and that the specification states this:

When performing a reverse mapping from SeverityNumber to a specific format and the SeverityNumber has no corresponding mapping entry for that format then it is recommended to choose the target severity that is in the same severity range and is closest numerically.

For example Zap has only one severity in the INFO range, called "Info". When doing reverse mapping all SeverityNumber values in INFO range (numeric 9-12) will be mapped to Zap’s "Info" level.

I feel like we should map all the DEBUG severity numbers to logging.DEBUG and so on for the rest of the severity numbers.

This piece of code is not related to reverse mapping, I didn't came across the need to use reverse mapping yet. This is used for mapping from python to OTLP format https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#mapping-of-severitynumber.

This piece of code is not related to reverse mapping, I didn't came across the need to use reverse mapping yet. This is used for mapping from python to OTLP format https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#mapping-of-severitynumber.

Ok, I see. What I feel arbitrary here is the multiple mapping of some integers to the same severity number (like 13, 14, 15... which are all mapped to DEBUG4). For this particular mapping between standard Python log levels to severity numbers, I think it makes more sense for the mapping function to accept only the numeric values of the standard logging levels instead.

BTW, it seems like this mapping function will be necessary for other third-party logging libraries as well? If that is the case, I think we should entry point this function too.

It may look arbitrary but it is not. Multiple severity numbers are mapped to same severity numbers because the gap between levels is 10 in python standard but whereas it 5 in OTLP. The higher number the more severity; levelno 13-19 may mean different severity in the context of python but when they should be mapped to OTLP we assign the max possible which is DEBUG4.

opentelemetry-sdk/src/opentelemetry/sdk/logs/export/__init__.py

ocelotl

Description

Unlike tracing and metrics, logs specification doesn't define new logging API. Many existing logging libraries have some sort of extension mechanism that allows to customize how log records are encoded and delivered to their destinations. The OpenTelemetry Logging Library SDK is intended to be used by such extensions to emit logs in OpenTelemetry formats. In Python this can be achieved with logging.Handlers and Python standard library provides many handlers out of the box.

Our LogEmitter implements the base Handler interface defined in logging module and emits the log events in OpenTelemetry manner.

Actually, logging.Handler is not an interface, it is a concrete class. This does not mean that a class can't inherit from logging.Handler but I wanted to point this out in case you are attempting to implement an interface completely by implementing all abstract methods.

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

srikanthccv · 2021-06-02T09:50:26Z

Correct, the use of term interface is technically incorrect. I meant to say we should just implement few methods in subclass https://docs.python.org/3/library/logging.html#logging.Handler.emit.

codeboten · 2021-06-03T16:58:10Z

See the spec pr to move the log OTLP protocol from experimental to beta: open-telemetry/opentelemetry-specification#1741

srikanthccv · 2021-06-04T16:52:21Z

@codeboten @lzchen @owais @ocelotl Following this discussion #1882 (comment) there will be need to make more changes. I think this PR is already big and adding more changes makes it difficult to review. I would like to close this and split into multiple PR each adding one component after other. What do you think?

ocelotl · 2021-06-04T17:10:15Z

@codeboten @lzchen @owais @ocelotl Following this discussion #1882 (comment) there will be need to make more changes. I think this PR is already big and adding more changes makes it difficult to review. I would like to close this and split into multiple PR each adding one component after other. What do you think?

Sure, if you find splitting this into smaller PRs to be more convenient, go ahead 👍

This is great work, I am excited to get this merged 💪

srikanthccv changed the base branch from main to experimental May 30, 2021 07:01

srikanthccv added 8 commits May 30, 2021 12:48

WIP: Logging SDK

fc07d85

Fix typos

065a54e

Add severity and update log record

36e9bff

Add unit tests

aa6df23

Add more unit tests

90e37c6

Add global provider and tests

01e9e97

mypy fixes

f1ce8f9

Add more docstrings

d34c64d

srikanthccv force-pushed the logs-sdk-experimental branch from cbb2ee4 to d34c64d Compare May 30, 2021 07:22

srikanthccv added 2 commits May 30, 2021 13:26

Fix lint issues

2dcebb2

more lint fixes

d8cc58c

srikanthccv mentioned this pull request May 30, 2021

[WIP] Logging SDK Prototype srikanthccv/opentelemetry-python#2

Closed

11 tasks

srikanthccv marked this pull request as ready for review June 1, 2021 02:47

srikanthccv requested review from a team, owais and ocelotl and removed request for a team June 1, 2021 02:47

owais reviewed Jun 1, 2021

View reviewed changes

ocelotl requested changes Jun 2, 2021

View reviewed changes

srikanthccv and others added 2 commits June 2, 2021 14:08

Rename log record

945bc48

Update opentelemetry-sdk/src/opentelemetry/sdk/logs/export/__init__.py

7b00de0

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

codeboten mentioned this pull request Jun 3, 2021

Add support for OpenTelemetry logging #1890

Closed

srikanthccv closed this Jun 5, 2021

wsargent mentioned this pull request Jul 21, 2021

Logging SDK open-telemetry/opentelemetry-java#3415

Closed

srikanthccv deleted the logs-sdk-experimental branch September 24, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial logging library SDK implementation #1882

Add initial logging library SDK implementation #1882

srikanthccv commented May 30, 2021 •

edited

Loading

srikanthccv commented Jun 1, 2021

owais Jun 1, 2021

srikanthccv Jun 1, 2021

ocelotl Jun 2, 2021

ocelotl Jun 2, 2021

ocelotl Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

srikanthccv Jun 3, 2021

ocelotl Jun 3, 2021

srikanthccv Jun 3, 2021

ocelotl Jun 3, 2021

srikanthccv Jun 4, 2021

ocelotl Jun 4, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl Jun 2, 2021

srikanthccv Jun 2, 2021

ocelotl left a comment

srikanthccv commented Jun 2, 2021

codeboten commented Jun 3, 2021 •

edited

Loading

srikanthccv commented Jun 4, 2021

ocelotl commented Jun 4, 2021 •

edited

Loading



		class LogData:
		"""Readable LogRecord data plus associated Resource and InstrumentationLibrary."""

Add initial logging library SDK implementation #1882

Add initial logging library SDK implementation #1882

Conversation

srikanthccv commented May 30, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

srikanthccv commented Jun 1, 2021

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

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

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

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

ocelotl left a comment

Choose a reason for hiding this comment

Description

srikanthccv commented Jun 2, 2021

codeboten commented Jun 3, 2021 • edited Loading

srikanthccv commented Jun 4, 2021

ocelotl commented Jun 4, 2021 • edited Loading

srikanthccv commented May 30, 2021 •

edited

Loading

codeboten commented Jun 3, 2021 •

edited

Loading

ocelotl commented Jun 4, 2021 •

edited

Loading