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

Add OTLPHandler for standard library logging module #1903

Merged
merged 13 commits into from
Jun 18, 2021

Conversation

srikanthccv
Copy link
Member

Description

Part-3 of #1890. Builds on top of #1901. This adds OTLPHandler for logging module. Python logging module provides a way to send the log records to the one or more appropriate destinations. More info about handler https://docs.python.org/3/library/logging.html#handler-objects

@srikanthccv srikanthccv marked this pull request as ready for review June 15, 2021 01:42
@srikanthccv srikanthccv requested a review from a team June 15, 2021 01:42
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Code looks good, just a typo and a question about naming. Ant thoughts on whether the OTLPHandler should be part of the SDK or a separate OTLP logging package?

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py Outdated Show resolved Hide resolved
@@ -111,6 +113,48 @@ def force_flush(self, timeout_millis: int = 30000):
"""


class OTLPHandler(logging.Handler):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called an emitter rather than a handler? Or is this a convenience handler to allow users to add this handler to their python root handler?

Copy link
Member Author

@srikanthccv srikanthccv Jun 16, 2021

Choose a reason for hiding this comment

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

Yes, this is one of the the handlers users can attach to root logger. I believe the name Handler makes more sense here.

@@ -121,6 +165,10 @@ def __init__(
self._resource = resource
self._instrumentation_info = instrumentation_info

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be on the provider level?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this should be on provider as well.

Co-authored-by: alrex <alrex.boten@gmail.com>
@srikanthccv
Copy link
Member Author

srikanthccv commented Jun 16, 2021

thoughts on whether the OTLPHandler should be part of the SDK or a separate OTLP logging package?

My initial thoughts were since this is Handler for logging module maybe we can provide this as a part of SDK and other third party libs as separate packages. No strong opinions.

@srikanthccv
Copy link
Member Author

One other argument to provide OTLPHandler as part of the SDK so that the users doesn’t need to find yet another separate package for something common.



_STD_TO_OTLP = {
10: SeverityNumber.DEBUG,
Copy link
Contributor

Choose a reason for hiding this comment

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

How come the interim levels defined like this? (11-19, 21-29, etc.)

Copy link
Contributor

@lzchen lzchen Jun 16, 2021

Choose a reason for hiding this comment

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

Curious will numbers in "std" ever be anything other than 0, 10, 20, 30, 40, 50?

Copy link
Member Author

Choose a reason for hiding this comment

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

This follows the log data model specification for mapping source format https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#mapping-of-severitynumber. The step value from one level to another level in logging module is 10 but the it is 5 in log data model, so it is clearly many-to-one mapping for some levels. Higher the value, higher the severity. In general these intermediary levels don't come into play but logging library allows programmers to define the custom log levels if they need, so this addresses that case. For example I would define some scary b/w error and critical level like below

>>> import logging
>>> SCARY = 45
>>> logging.addLevelName(SCARY, "SCARY")
>>> def scary(self, message, *args, **kws):
...     self._log(SCARY, message, args, **kws)
...
>>> logging.Logger.scary = scary
>>> logger = logging.getLogger(__name__)
>>> logger.scary("Boo")
Boo

def _translate(self, record: logging.LogRecord) -> LogRecord:
timestamp = int(record.created * 1e9)
span_context = get_current_span().get_span_context()
# TODO: attributes (or resource attributes?) from record metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I believe attributes can be populated from both custom and resources.

Copy link
Member Author

Choose a reason for hiding this comment

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

So logging.LogRecord https://docs.python.org/3/library/logging.html#logrecord-attributes has a number of attributes like funcName, filename, and processName etc.. I was wondering if we should just make these as OTLP log record attributes https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes or log record resource attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

For logging.LogRecord attributes, it makes more sense to be part of the OTLP log record attributes.

When I was saying "custom" attributes, I meant the ability for users to add custom attributes to the LogRecord themselves (it is a feature in OpenCensus as well), but may not be defined yet in the specs.

Not any concern for now, just something to be mindful of.

Copy link
Contributor

Choose a reason for hiding this comment

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

logging.LogRecord has an (optional) extra attribute that holds arbitrary user-injected attributes.
This is not super well documented, but the docs can be found a couple paragraphs down from here.

From a users perspective, it looks like this:

logging.debug("msg", extra={...})

Would it be possible to map this to OTLP log record attributes? You could take the top-level attributes of extra (which would be my preference), or require them to be nested under a specific key (e.g. logging.debug("msg", extra={"oltp-attributes": {...}})).

self._log_emitter = log_emitter or get_log_emitter(__name__)

def _translate(self, record: logging.LogRecord) -> LogRecord:
timestamp = int(record.created * 1e9)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for your information, this PEP may be relevant here.

@lzchen lzchen merged commit cdea25a into open-telemetry:logs Jun 18, 2021
@srikanthccv srikanthccv deleted the logging-otlp-handler branch September 24, 2021 08:39
lzchen pushed a commit to lzchen/opentelemetry-python that referenced this pull request Oct 15, 2021
owais added a commit that referenced this pull request Nov 3, 2021
* Add initial overall structure and classes for logs sdk (#1894)

* Add global LogEmitterProvider and convenience function get_log_emitter (#1901)

* Add OTLPHandler for standard library logging module (#1903)

* Add LogProcessors implementation (#1916)

* Fix typos in test_handler.py (#1953)

* Add support for OTLP Log exporter (#1943)

* Add support for user defined attributes in OTLPHandler (#1952)

* use timeout in force_flush (#2118)

* use timeout in force_flush

* fix lint

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

Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>

* fix lint

Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>

* add a ConsoleExporter for logging (#2099)

Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>

* Update SDK docs and Add example with OTEL collector logging (debug) exporter (#2050)

* Fix exception in severity number transformation (#2208)

* Fix exception with warning message transformation

* Fix lint

* Fix lint

* fstring

* Demonstrate how to set the Resource for LogEmitterProvider (#2209)

* Demonstrate how to set the Resource for LogEmitterProvider

Added a Resource to the logs example to make it more complete.
Previously it was using the built-in Resource. Now it adds the
service.name and service.instance.id attributes.

The resulting emitted log records look like this:
```
Resource labels:
     -> telemetry.sdk.language: STRING(python)
     -> telemetry.sdk.name: STRING(opentelemetry)
     -> telemetry.sdk.version: STRING(1.5.0)
     -> service.name: STRING(shoppingcart)
     -> service.instance.id: STRING(instance-12)
InstrumentationLibraryLogs #0
InstrumentationLibrary __main__ 0.1
LogRecord #0
Timestamp: 2021-10-14 18:33:43.425820928 +0000 UTC
Severity: ERROR
ShortName:
Body: Hyderabad, we have a major problem.
Trace ID: ce1577e4a703f42d569e72593ad71888
Span ID: f8908ac4258ceff6
Flags: 1
```

* Fix linting

* Use batch processor in example (#2225)

* move logs to _logs (#2240)

* move logs to _logs

* fix lint

* move log_exporter to _log_exporter as it's still experimental (#2252)

Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Leighton Chen <lechen@microsoft.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Owais Lone <owais@users.noreply.github.com>
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants