[logs-sdk] Remove LogData and extend SDK LogRecord to have instrumentation scope #4676
Conversation
|
Contrib tests expect an object like log.log_record returned in log_exporter.get_finished_logs() |
|
What if instead of removing I don't think we need a This way we don't need to update 2 classes every time a change is made to the log data structure. Also I think our implementation is confusing the 2 classes in a bunch of places maybe because they are named the same. For example I think |
|
@DylanRussell we can also call it SDKLogRecord like Javascript https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/sdk-logs/src/index.ts#L24 |
|
Yeah I think i'm OK with that name too. But what do you think of it being just the 3 fields I mentioned above ? And switching Logger.emit in the SDK to take the API LogRecord and then create the SDK LogRecord and attach instrumentation_scope/resource to it.. That looks to be what javascript is doing too: |
| @@ -45,6 +45,8 @@ can cause a deadlock to occur over `logging._lock` in some cases ([#4636](https: | |||
| ([#4669](https://github.com/open-telemetry/opentelemetry-python/pull/4669)) | |||
| - Set expected User-Agent in HTTP headers for grpc OTLP exporter | |||
| ([#4658](https://github.com/open-telemetry/opentelemetry-python/pull/4658)) | |||
| - Remove LogData and extend SDK LogRecord to have instrumentation scope | |||
There was a problem hiding this comment.
We should have a breaking changes section or something here, maybe I can update the title of the PR to reflect that
| @@ -49,13 +48,9 @@ def emit(self, event: Event) -> None: | |||
| if isinstance(self._logger, NoOpLogger): | |||
| # Do nothing if SDK is disabled | |||
| return | |||
| span_context = trace.get_current_span().get_span_context() | |||
There was a problem hiding this comment.
Context is not being passed anymore, but this code is going away so not sure if we want to add it
|
@DylanRussell updated, still having SDKLogRecord inheriting API LogRecord, having the API LogRecord as an attribute feels really weird to me, I think it could cause more confusion an issues than solving them, let me know what you think. |
| @@ -210,48 +195,20 @@ def __init__( | |||
| event_name: str | None = None, | |||
| ): ... | |||
|
|
|||
| @overload | |||
| @deprecated( | |||
There was a problem hiding this comment.
This is changing the actual class name so I think is a good opportunity to get rid of previously deprecated parameters
|
@open-telemetry/python-maintainers this is definitely a big breaking change, do we want to use another branch to group the Logs updates together?, we discussed this last week SIG meeting, but trying to understand what is the best way to move forward here. |
|
In my opinion just have SdkLogRecord with a ApiLogRecord attribute, it should not inherit from it at all now.. We don't need another class with all the same attributes. We just need a small wrapper class..
And |
|
Looks like some instrumentations fail w/ this change (https://github.com/open-telemetry/opentelemetry-python-contrib/blob/f20fa77ad59d90aae4978ae28cb1a98b12fbb959/instrumentation-genai/opentelemetry-instrumentation-vertexai/tests/test_function_calling.py#L4 -- this instrumentation is accessing |
Description
Removing LogData and extending SDK LogRecord to have instrumentation scope
Fixes #4313
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