-
Notifications
You must be signed in to change notification settings - Fork 606
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
Fix otlp exporter translating sequence types #1818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the PR and thanks to @LouisStAmour for the issue and code to address it. I'm requesting change until the discussion around what to do w/ Mapping types is resolved.
exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py
Outdated
Show resolved
Hide resolved
@@ -545,6 +550,55 @@ def test_span_status_translate(self): | |||
Status.DEPRECATED_STATUS_CODE_UNKNOWN_ERROR, | |||
) | |||
|
|||
# pylint:disable=no-member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tests makes me think we should make use to pytest parametrization https://docs.pytest.org/en/6.2.x/parametrize.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a possible improvement however it might be a bit complex due to the nested property checking that this specific test is doing. I can see it being useful for other tests that have code branches like this one though.
|
||
# Tracing specs currently does not support Mapping type attributes | ||
elif isinstance(value, Mapping): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have an issue for this in the spec. Translating a map to a list of KeyValues should be defined in the spec, otherwise there's no guarantee of what other languages will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this logic branch since mapping types cannot be added as attributes currently, as defined in the tracing spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Left a question around support of mapping types for attribute values.
# _translate_key_values(str(k), v) for k, v in value.items() | ||
# ] | ||
# ) | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean any spans containing a Mapping attribute will throw an exception below? I don't think we should do that if the API allows to create KV attributes (not sure if it does). May be we can warn and drop the attribute instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API does not allow adding an attribute that is of type Mapping (https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L67)
Fixes #1755
As of today, span attributes cannot be a mapping type, so the logic for translating mapping types should never be hit.
Proto supports KeyValueList, which might be ahead of the tracing spec in terms of data types that it supports.
Thanks @LouisStAmour for the code snippet.