Skip to content
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

AttributeValues accepting a list could lead to invalid types being added #352

Closed
toumorokoshi opened this issue Jan 3, 2020 · 7 comments
Assignees
Labels

Comments

@toumorokoshi
Copy link
Member

#348 is tackling validation for attribute values, of which list is one of them. However, lists can be mutated as long as someone has a handle, so one could potentially misuse the Span api by mutating lists afterward.

The purpose of adding validation during the setting of span attributes was to remove the need to perform such validations in the exporters themselves.

Should we freeze lists as they come in? or should we potentially just allow tuples rather than lists as attribute values?

@ocelotl
Copy link
Contributor

ocelotl commented Jan 3, 2020

We could accept any sequence and copy all of its values in a tuple when we store them.

Does the OpenTelemetry specification says anything about sequences that hold sequences themselves?

@ocelotl ocelotl added the discussion Issue or PR that needs/is extended discussion. label Jan 3, 2020
@jakemalachowski
Copy link
Contributor

@ocelotl I was wondering that when implementing attribute validation. The OT spec says:

The attribute value, which is either:
A primitive type: string, boolean or numeric.
An array of primitive type values. The array MUST be homogeneous, i.e. it MUST NOT contain values of different types.

Since the array values must all be of a primitive values, this seems to indicate that a list of lists is invalid.

@ocelotl
Copy link
Contributor

ocelotl commented Jan 7, 2020

Correct, there should not be sequences of sequences. 👍

@Oberon00
Copy link
Member

I don't think we need to make the SDK completely fool-proof. Checking list values on every set_attribute sounds rather expensive.

@toumorokoshi toumorokoshi added good first issue Good first issue discussion Issue or PR that needs/is extended discussion. and removed discussion Issue or PR that needs/is extended discussion. good first issue Good first issue labels Jan 14, 2020
@toumorokoshi
Copy link
Member Author

@Oberon00 looks like you're looking at #348, which is a good venue to discuss the choice of validation at all.

looks like there's roughly a 183ns cost to including this in the trace construction, at the gain of immutability. I think there's merit to make this change:

# On an Intel i7-3820
$ python -m timeit "tuple(['a','b','c'])"
10000000 loops, best of 3: 0.183 usec per loop

I'll put help wanted on this, as it's a good starter task.

@toumorokoshi toumorokoshi added good first issue Good first issue and removed discussion Issue or PR that needs/is extended discussion. labels Jan 14, 2020
@AndrewAXue
Copy link
Contributor

I can take this

@AndrewAXue
Copy link
Contributor

codeboten pushed a commit that referenced this issue May 15, 2020
This PR addresses issue #352

Validates attribute values and events upon span creation and setting attributes on spans.

Validation logic involves checking if value is one of (int, float, str, bool, list), and if list, each element must be valid and must be all the same primitive, valid type
list values will not contain nested lists
If value is a list, grants immutable property by converting value to a tuple.
cnnradams pushed a commit to cnnradams/opentelemetry-python that referenced this issue May 15, 2020
This PR addresses issue open-telemetry#352

Validates attribute values and events upon span creation and setting attributes on spans.

Validation logic involves checking if value is one of (int, float, str, bool, list), and if list, each element must be valid and must be all the same primitive, valid type
list values will not contain nested lists
If value is a list, grants immutable property by converting value to a tuple.
owais pushed a commit to owais/opentelemetry-python that referenced this issue May 22, 2020
This PR addresses issue open-telemetry#352

Validates attribute values and events upon span creation and setting attributes on spans.

Validation logic involves checking if value is one of (int, float, str, bool, list), and if list, each element must be valid and must be all the same primitive, valid type
list values will not contain nested lists
If value is a list, grants immutable property by converting value to a tuple.
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
closes open-telemetry#351

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants