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

added readme.rst file log record enrichment behavior of logging instr… #2904

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sunilwali679
Copy link

@Sunilwali679 Sunilwali679 commented Oct 15, 2024

Fixes #2642

@Sunilwali679
Copy link
Author

Hi @lzchen , Could you please review and approve .

@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 17, 2024
Log record enrichment behavior of logging instrumentation

"""
The OpenTelemetry ``logging`` integration automatically injects tracing context into log statements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use instrumentation as the component name


"""
The OpenTelemetry ``logging`` integration automatically injects tracing context into log statements.
It's structured within an `if` statement that checks [`set_logging_format`] is `True`, indicating that logging configuration should be applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think implementation details like this is necessary to include in the README

The OpenTelemetry ``logging`` integration automatically injects tracing context into log statements.
It's structured within an `if` statement that checks [`set_logging_format`] is `True`, indicating that logging configuration should be applied.

The integration registers a custom log record factory with the the standard library logging module that automatically inject
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

tracing context into log record objects. Optionally, the integration can also call ``logging.basicConfig()`` to set a logging
format with placeholders for span ID, trace ID and service name.

1. **Determining the Log Format**: The code first attempts to determine the logging format by looking for a `logging_format` key in the [`kwargs`]( dictionary. If it's not found, it tries to fetch the value from an environment variable named [`OTEL_PYTHON_LOG_FORMAT`]. If neither is available, it defaults to a predefined constant [`DEFAULT_LOGGING_FORMAT`]. This approach provides flexibility, allowing the log format to be specified through function arguments, environment variables, or falling back to a default if neither is provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this level of detail for implementation. I believe it is enough to have the related env vars included in the README (as you have below).




API
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the API section as the first section before env vars

LoggingInstrumentor().instrument(set_logging_format=True)


Note
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note
Setting logging format outside of the instrumentation


The default value is ``info``.

Options are:
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to mention these as these are standard python logging library values.

LoggingInstrumentor(log_level=logging.DEBUG)


The default value is ``info``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The default value is ``info``.
The default value is ``logging.INFO``.


.. code-block::

{default_logging_format}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have a separate section for the default format. Simply paste it into the below section regarding OTEL_PYTHON_LOG_FORMAT env var


.. code-block::

{default_logging_format}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to link out, just paste the format here.


.. envvar:: OTEL_PYTHON_LOG_CORRELATION

This env var must be set to ``true`` in order to enable trace context injection into logs by calling ``logging.basicConfig()`` and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This env var must be set to ``true`` in order to enable trace context injection into logs by calling ``logging.basicConfig()`` and
This env var must be set to ``true`` in order to enable trace context injection into logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't include implementation detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document log record enrichment behavior of logging instrumentation
3 participants