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

LoggingInstrumentation: Set Handler Format Explictly If basicConfig Has Already Been Called #1939

Closed
wants to merge 3 commits into from

Conversation

rjduffner
Copy link
Contributor

Description

We are using open telemetry with Gunicorn. Due to the forking model of Gunicorn (https://opentelemetry-python.readthedocs.io/en/latest/examples/fork-process-model/README.html), we are unable to do automatic instrumentation.

Since Gunicorn sets up logging very early, setting the logFormat via OTEL_PYTHON_LOG_CORRELATION and OTEL_PYTHON_LOG_FORMAT doesn't do anything. This causes us to have to set the log format to include the trace_id/span_id in the generic formatter. This means that the LoggingInstrumention always has to be instrumented even locally.

This is a first draft of a potential way to fix it. I am looking for some feedback.

The Fix:

  • Since the handler is created the first time one of the configs is called, we need to update the handler's format in place to use the instrumented logger.

The Open Questions:

  • Most example code where people update the formatter in place assumes there is only one handler defined. While this is ok for most things, there might be cases where multiple handlers are defined. I chose to iterate over them all and change each formatter. This could have some unintended consequences though. Beyond, specifying which handler to use (or which format to override specifically), I am not sure of a better method.

The Annoying:

  • Since the instrumentations are instrumented in alphabetical order, only logs produced after the logging instrumentation is instrumented get the new format. This means that the format changes during startup which is not ideal.
2023-09-06 11:44:40,943 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented boto3
2023-09-06 11:44:40,946 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented botocore
2023-09-06 11:44:40,955 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented confluent_kafka
2023-09-06 11:44:40,958 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented grpc_aio_client
2023-09-06 11:44:40,959 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented grpc_aio_server
2023-09-06 11:44:40,959 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented grpc_client
2023-09-06 11:44:40,960 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented grpc_server
2023-09-06 11:44:40,974 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented jinja2
2023-09-06 11:44:40,997 DEBUG [o11y.tracing:118][pid=19962][MainThread] Instrumented kafka
2023-09-06 11:44:40,999 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented logging
2023-09-06 11:44:41,009 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented pymemcache
2023-09-06 11:44:41,042 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented pyramid
2023-09-06 11:44:41,060 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented redis
2023-09-06 11:44:41,076 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented requests
2023-09-06 11:44:41,083 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented sqlite3
2023-09-06 11:44:41,085 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented urllib
2023-09-06 11:44:41,087 DEBUG [o11y.tracing] [tracing.py:118] [trace_id=0 span_id=0 resource.service.name=starfish_local trace_sampled=False] - Instrumented urllib3

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Tested locally with a sample Gunicorn app until I get some guidance on final functionality.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@dhofstetter
Copy link
Contributor

There are two open issues within the opentelemetry-python repository that are affected by this pr.

What can we do helping to get this pr reviewed and merged?

@howardyoo
Copy link

There are two open issues within the opentelemetry-python repository that are affected by this pr.

What can we do helping to get this pr reviewed and merged?

Add this issue on top of those two: #2346

root_logger = logging.getLogger()
if root_logger.hasHandlers():
for handler in root_logger.handlers:
handler.setFormatter(logging.Formatter(log_format))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about log_level?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ocelotl Is there a reason to review this pr and not the more recent one here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ocelotl, as @dhofstetter, said lets go with their newer and more complete pr. i meant this as more a discussion starter.

thanks for the review though.

also ill close this pr so its not confusing anymore

@rjduffner
Copy link
Contributor Author

going to close this pr. @dhofstetter has a more complete implementation. thanks @dhofstetter and @ocelotl

@rjduffner rjduffner closed this May 11, 2024
@rjduffner rjduffner deleted the loggingBasicConfig branch May 11, 2024 04:10
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.

4 participants