fix AttributePropagatingSpanProcessor with NonRecordingSpan as parent

[zhongkechen]

Issue #, if available:

Fixes: #743

Description of changes:

aws-otel-python-instrumentation/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/attribute_propagating_span_processor.py

Lines 80 to 81 in 28fc34e

	elif parent_span and _is_server_kind(parent_span):
	propagation_data = self._propagation_data_extractor(parent_span)

When the parent_span isn't a ReadableSpan (e.g. a NonRecordingSpan), _is_server_kind would throw an attribute exception because the other types of span may not have the kind attribute.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[liustve]

I partially am okay with these changes, though I don't agree with this span processor working with NonRecording Spans since there's no SpanKind.
https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/span.py#L523

[liustve]

instead of isinstance(parent_span, ReadableSpan), could we use parent_span.is_recording() instead?

The OTel spec defines that NonRecordingSpan.is_recording() must return False, so it's a cleaner check that doesn't require importing ReadableSpan and aligns directly with the spec's contract:

        elif parent_span and parent_span.is_recording() and _is_server_kind(parent_span):
            propagation_data = self._propagation_data_extractor(parent_span)
        elif parent_span and parent_span.is_recording():
            propagation_data = parent_span.attributes.get(self._propagation_data_key)

[liustve]

@zhongkechen Please add unit tests for this change as well and a CHANGELOG.md entry

[zhongkechen]

instead of isinstance(parent_span, ReadableSpan), could we use parent_span.is_recording() instead?

The OTel spec defines that NonRecordingSpan.is_recording() must return False, so it's a cleaner check that doesn't require importing ReadableSpan and aligns directly with the spec's contract:

        elif parent_span and parent_span.is_recording() and _is_server_kind(parent_span):
            propagation_data = self._propagation_data_extractor(parent_span)
        elif parent_span and parent_span.is_recording():
            propagation_data = parent_span.attributes.get(self._propagation_data_key)

I'm using isinstance(parent_span, ReadableSpan) because a few lines above my change is using that:

https://github.com/zhongkechen/aws-otel-python-instrumentation/blob/0ac556982b8a06e456018e3dbd2948898dfe96e7/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/attribute_propagating_span_processor.py#L50

    def on_start(self, span: Span, parent_context: Optional[Context] = None) -> None:
        parent_span: ReadableSpan = get_current_span(parent_context)
        if isinstance(parent_span, ReadableSpan):

I thought I should keep the check logic consistent at least in the same method

[liustve]

instead of isinstance(parent_span, ReadableSpan), could we use parent_span.is_recording() instead?
The OTel spec defines that NonRecordingSpan.is_recording() must return False, so it's a cleaner check that doesn't require importing ReadableSpan and aligns directly with the spec's contract:

        elif parent_span and parent_span.is_recording() and _is_server_kind(parent_span):
            propagation_data = self._propagation_data_extractor(parent_span)
        elif parent_span and parent_span.is_recording():
            propagation_data = parent_span.attributes.get(self._propagation_data_key)

I'm using isinstance(parent_span, ReadableSpan) because a few lines above my change is using that:

https://github.com/zhongkechen/aws-otel-python-instrumentation/blob/0ac556982b8a06e456018e3dbd2948898dfe96e7/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/attribute_propagating_span_processor.py#L50

    def on_start(self, span: Span, parent_context: Optional[Context] = None) -> None:
        parent_span: ReadableSpan = get_current_span(parent_context)
        if isinstance(parent_span, ReadableSpan):

I thought I should keep the check logic consistent at least in the same method

I see though I'd still suggest using what is OTel spec aligned instead of using isinstance imo.

[Aneurysm9]

I don't think parent_span.is_recording() actually conveys the necessary information here. That's an API method intended for allowing instrumentation to skip potentially expensive attribute calculation/creation operations if the result won't be recorded. An API span may be recording and still write-only. Also, OnStart() should only ever be called on recording spans and I'm not aware of any situation in which a recording span would have a non-recording parent span. Maybe if the parent span was ended before the child was started, but even then it may be possible to get a readable span from the parent despite it no longer being in a recording state.

Since this is an SDK plugin it should be fine for it to introspect API spans to get at the underlying SDK spans, which is where ReadableSpan would fit in. The thing I'm not sure of with the Python implementation is whether a ReadableSpan is always read-only, or if a read/write span also satisfies isinstance(ps, ReadableSpan).

[liustve]

I don't think parent_span.is_recording() actually conveys the necessary information here. That's an API method intended for allowing instrumentation to skip potentially expensive attribute calculation/creation operations if the result won't be recorded. An API span may be recording and still write-only. Also, OnStart() should only ever be called on recording spans and I'm not aware of any situation in which a recording span would have a non-recording parent span. Maybe if the parent span was ended before the child was started, but even then it may be possible to get a readable span from the parent despite it no longer being in a recording state.

Since this is an SDK plugin it should be fine for it to introspect API spans to get at the underlying SDK spans, which is where ReadableSpan would fit in. The thing I'm not sure of with the Python implementation is whether a ReadableSpan is always read-only, or if a read/write span also satisfies isinstance(ps, ReadableSpan).

I'm okay with the changes then, @zhongkechen please add unit tests and a changelog entry and we can help merge this

[zhongkechen]

added a unit test case