Move logger handlers to opentelemetry-instrumentation-logging#4210
Move logger handlers to opentelemetry-instrumentation-logging#4210xrmx wants to merge 8 commits intoopen-telemetry:mainfrom
Conversation
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
There was a problem hiding this comment.
This file contains stuff that previously was in the sdk _logs and _configuration modules
pmcollins
left a comment
pmcollins
left a comment
There was a problem hiding this comment.
Looks nice -- added a couple of questions.
|
|
||
| enabled_logging_integrations = [] | ||
| if ( | ||
| environ.get(_OTEL_PYTHON_LOGGING_AUTO_INSTRUMENTATION_ENABLED, "false") |
There was a problem hiding this comment.
Should this env var be just for the sdk version of the logging handler? That way both implementations can co-exist until the sdk one eventually gets removed. And both during the transition period and afterwards, users can control whether this handler is enabled by whether this package is installed, just like other instrumentation packages.
There was a problem hiding this comment.
I've ripped that code out in open-telemetry/opentelemetry-python#4919
But yeah we can add that code back but we need to disable this one if we found that the env var is set to true.
There was a problem hiding this comment.
Reverted the changes in core, per your suggestion.
| logging = "opentelemetry.instrumentation.logging:LoggingInstrumentor" | ||
|
|
||
| [project.entry-points.opentelemetry_logging_integrations] | ||
| logging = "opentelemetry.instrumentation.logging.handler:_setup_logging_handler" |
There was a problem hiding this comment.
Should the handler entrypoints look like the instrumentor entry points? e.g. singular instead of plural and maybe refer to a class rather than a function. So since the instrumentation entry points look like this:
[project.entry-points.opentelemetry_instrumentor]
aio-pika = "opentelemetry.instrumentation.aio_pika:AioPikaInstrumentor"
log handlers could be set up like:
[project.entry-points.opentelemetry_logging_initializer]
stdlib = "opentelemetry.instrumentation.logging:LoggingHandlerInitializer"
but now I'm wondering why we need a new entrypoint group at all. I realize this was discussed in the issue, but can handler setup be done in a plain old instrumentor? 🤔
There was a problem hiding this comment.
We haven't discussed that, this is a proposal :) The issue here is handling co-existence with the one in sdk in both manual and auto instrumentation scenarios.
There was a problem hiding this comment.
Dropped the new entry point group and moved the setup to the instrumentor. Thanks!
xrmx
changed the title
| from opentelemetry._logs import get_logger as APIGetLogger | ||
| from opentelemetry.attributes import BoundedAttributes | ||
| from opentelemetry.sdk import trace | ||
| from opentelemetry.sdk._logs import ( |
2 participants
Description
Draft PR for sketching LoggingHandler move outside of sdk.
Refs open-telemetry/opentelemetry-python#4330
Type of change
Please delete options that are not relevant.
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
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.