Requesting TC review for Python Logs API/SDK spec compliance

[jeremydvoss]

I have been investigating if Python's logging signal is ready to be stabilized. Could we get a TC member to look through the code and confirm that it is in line with the log spec and sem conv?

[UPDATE from @tigrannajaryan] Checklist to verify, borrowed from https://github.com/open-telemetry/community/issues/1537 (checkmark means reviewed/verified against spec definition):

• [ ] Getting Started example works
  • https://github.com/open-telemetry/opentelemetry-python/issues/3551
• [x] LoggerProvider.Get Logger
  • Minor doc issues - https://github.com/open-telemetry/opentelemetry-python/issues/4318
• [x] LoggerProvider.Get Logger accepts attributes
  • Minor doc issues - https://github.com/open-telemetry/opentelemetry-python/issues/4318
• [ ] LoggerProvider.Shutdown
  • https://github.com/open-telemetry/opentelemetry-python/issues/4317
• [x] LoggerProvider.ForceFlush
• [x] Can get global LoggerProvider
• [x] Can set global LoggerProvider
• [ ] Logger.Emit(LogRecord)
  • https://github.com/open-telemetry/opentelemetry-python/issues/4319
  • https://github.com/open-telemetry/opentelemetry-python/issues/4315
• [x] SimpleLogRecordProcessor
• [x] BatchLogRecordProcessor
• [x] Can plug custom LogRecordProcessor
• [x] OTLP/gRPC exporter
• [x] OTLP/HTTP exporter
• [ ] OTLP File exporter - According to compatibility matrix, not yet implemented.
• [x] Can plug custom LogRecordExporter
  • https://github.com/open-telemetry/opentelemetry-python/issues/4321
• [x] Trace Context Injection
  • yes, but only when using LoggingHandler bridge
  • https://github.com/open-telemetry/opentelemetry-python/issues/4319
• [ ] LogRecord fields output
  • https://github.com/open-telemetry/opentelemetry-python/issues/4322

[tigrannajaryan]

From what I understand this is a request for a TC review of Python Logging implementation. I will bring this up in the next TC meeting.

[tigrannajaryan]

@open-telemetry/python-maintainers can you please also confirm the review request?

[ocelotl]

Yes, @tigrannajaryan this is our request, thanks!

[tigrannajaryan]

@ocelotl thanks. I will confirm in the next TC meeting and will post back here who will do the review.

[tigrannajaryan]

Update: No TC meeting this week due to Kubecon, will be discussed next week.

[tigrannajaryan]

I will be doing the review.

[tigrannajaryan]

Trace Context Injection works as expected, I can see a span and a log record in its context:

{
    "name": "roll",
    "context": {
        "trace_id": "0x8b346ee9af8718bf73a4318d073cbbb1",
        "span_id": "0x7d0581e1f3f51bea",
        "trace_state": "[]"
    },
    "kind": "SpanKind.INTERNAL",
    "parent_id": null,
    "start_time": "2023-11-22T17:01:36.027543Z",
    "end_time": "2023-11-22T17:01:36.027925Z",
    "status": {
        "status_code": "UNSET"
    },
    "attributes": {
        "roll.value": "5"
    },
    "events": [],
    "links": [],
    "resource": {
        "attributes": {
            "telemetry.sdk.language": "python",
            "telemetry.sdk.name": "opentelemetry",
            "telemetry.sdk.version": "1.21.0",
            "service.name": "dice-server",
            "telemetry.auto.version": "0.42b0"
        },
        "schema_url": ""
    }
}
{
    "body": "Anonymous player is rolling the dice: 5",
    "severity_number": "<SeverityNumber.WARN: 13>",
    "severity_text": "WARNING",
    "attributes": {
        "otelSpanID": "7d0581e1f3f51bea",
        "otelTraceID": "8b346ee9af8718bf73a4318d073cbbb1",
        "otelTraceSampled": true,
        "otelServiceName": "dice-server"
    },
    "dropped_attributes": 0,
    "timestamp": "2023-11-22T17:01:36.027744Z",
    "trace_id": "0x8b346ee9af8718bf73a4318d073cbbb1",
    "span_id": "0x7d0581e1f3f51bea",
    "trace_flags": 1,
    "resource": "BoundedAttributes({'telemetry.sdk.language': 'python', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': '1.21.0', 'service.name': 'dice-server', 'telemetry.auto.version': '0.42b0'}, maxlen=None)"
}

[tigrannajaryan]

OTLP File exporter is not yet implemented, according to the compatibility matrix, so skipping the step.

[tigrannajaryan]

LoggerProvider.ForceFlush verified, works as expected. Code used:

logger2.warning("Jail zesty vixen who grabbed pay from quack.")

# Trace context correlation
tracer = trace.get_tracer(__name__)
with tracer.start_as_current_span("foo"):
    # Do something
    logger2.error("Hyderabad, we have a major problem.")

logger_provider.force_flush()

time.sleep(10)

logger_provider.shutdown()

Logs are printed immediately, when logger_provider.force_flush() is executed. Without force_flush() they get delayed until the batch processor decides to output them.

[tigrannajaryan]

@ocelotl @open-telemetry/python-maintainers I started the review and will be filing issues like this as I go: https://github.com/open-telemetry/opentelemetry-python/issues/created_by/tigrannajaryan

Let me know if you want me to ping you on each issue.

[garry-cairns]

Hi: keen to get logging to stable here so please let me know if I can help out in any way

[tigrannajaryan]

Hi: keen to get logging to stable here so please let me know if I can help out in any way

@garry-cairns Thank you. It would be great if you can help fix any of the non-compliance issues that I filed so far: https://github.com/open-telemetry/opentelemetry-python/issues/created_by/tigrannajaryan

[tigrannajaryan]

@open-telemetry/python-maintainers FYI, I am waiting for you to respond/resolve issues I opened so far so that I can continue the review.

[lzchen]

@tigrannajaryan

It is in our timelines to address these issues. Just facing other priorities at the moment. @garry-cairns Feel free to pickup any of the tasks if you want to expedite the process.

@jeremydvoss fyi

[garry-cairns]

@tigrannajaryan

It is in our timelines to address these issues. Just facing other priorities at the moment. @garry-cairns Feel free to pickup any of the tasks if you want to expedite the process.

@jeremydvoss fyi

Yep I've commented on 3545 and 3546 to say I don't think they reproduce, and I've sent a PR I believe will close out 3060 if you can take a look when you have a moment.