OTLP max_attr_value_length option #1824
Conversation
owais
requested review from
a team,
aabmass and
ocelotl
and removed request for
a team
owais
force-pushed
the
otlp-max-attr-length
branch
3 times, most recently
from
e91072a to
af21980
Compare
codeboten
changed the title
...porter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py
Outdated
Show resolved
Hide resolved
| @@ -81,12 +81,16 @@ def environ_to_compression(environ_key: str) -> Optional[Compression]: | |||
| return _ENVIRON_TO_COMPRESSION[environ_value] | |||
|
|
|||
|
|
|||
| def _translate_key_values(key: Text, value: Any) -> KeyValue: | |||
| def _translate_key_values( | |||
| key: Text, value: Any, max_attr_value_length: Optional[int] | |||
There was a problem hiding this comment.
| key: Text, value: Any, max_attr_value_length: Optional[int] | |
| key: Text, value: Any, max_attr_value_length: Optional[int] = None |
There was a problem hiding this comment.
So I wanted it to be optional so other places could explicitly pass None but still wanted to force users to always pass a value. This was to ensure that any OTLP exporter does not forget to include the length limit in some invocations.
There was a problem hiding this comment.
I'm not sure I understand the explanation. Did you mean you wanted it to be explicitly passed in to _translate_key_values when used internally, but when using the OTLP exporter, users are not forced to pass it in?
There was a problem hiding this comment.
Since this is a private function, I don't expect or would like to not account for users using this function.
What I meant was that it is very easy for a contributor to call this function somewhere without forwarding/passing the limit value if we keep it as an optional paramater. Since this function can be called from so many different places, it's easy for a contributor to forget to pass the limit value and for reviewers to not catch that. Making it a required param would catch all such mistakes while still allowing any possible use cases where limit is not desired (by setting it to None).
owais
force-pushed
the
otlp-max-attr-length
branch
from
af21980 to
ebaae08
Compare
|
|
||
| if isinstance(value, bool): | ||
| any_value = AnyValue(bool_value=value) | ||
|
|
||
| elif isinstance(value, str): | ||
| if max_attr_value_length is not None: |
There was a problem hiding this comment.
I think this applies for the sequence type as well.
| @@ -70,6 +70,7 @@ class OTLPSpanExporter( | |||
| headers: Headers to send when exporting | |||
| timeout: Backend request timeout in seconds | |||
| compression: gRPC compression method to use | |||
| max_attr_value_length: Max length string attribute values can have. Set to None to disable. | |||
There was a problem hiding this comment.
I think it applies to sequence attributes too. Although the behavior for zipkin and jaeger truncates the sequence in a stringified form, so not sure when this would happen for otlp.
There was a problem hiding this comment.
It does but I saw your PR where you are calling the translate func on each sequence item so that should take care of this use case.
There was a problem hiding this comment.
But if I remember it incorrectly, happy to do it here.
There was a problem hiding this comment.
It does but I saw your PR where you are calling the translate func on each sequence item so that should take care of this use case.
Ahh I see. I think my PR recursively calls this on the sequence VALUES and truncates them if they are strings. However, the behaviour of jaeger and zipkin first stringifies the sequence and then applies the truncating on the resulting string. Doesn't stringifying a sequence (like a tuple) makes it include the "(", thus contributing to the length?
On another note, if you could review this since it is a blocker that would be great :)
There was a problem hiding this comment.
Yea, I'm pretty sure jaeger and zipkin implementations are wrong according to the spec. I'll create an issue to fix them.
There was a problem hiding this comment.
The proposed addition to spec recommends to truncate individual values in sequence types. The proposal has not been merged yet but judging from the discussion I don't see it changing drastically. (open-telemetry/opentelemetry-specification#1130)
Either way we can use it as a general guidance for this feature when implementing in exporters. Once it is merged, we'd likely want to implement these limits directly in the span or attribute implementations.
There was a problem hiding this comment.
Yeah truncating individual values makes much more sense.
|
@lzchen I think this proposal will be merged soon. I do need this feature in Splunk distribution kind of urgently but I don't think worth adding a new parameter to the OTLP exporter in case we end up implementing this more centrally in the SDK like other limits. So I'm closing this for now and waiting for the spec proposal. Will re-implement it in SDK once the proposal is merged. |
Description
Added
max_attr_value_lengthoption to OTLP exporter just like Zipkin and Jaeger exporters support already.Fixes # (issue)
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 Contrib Repo Change?
Checklist: