Freeze sequence-valued span attributes

[c24t]

Re-implements #401 as the original author didn't sign the CLA before going MIA.

[codecov-io]

Codecov Report

Merging #449 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #449   +/-   ##
=======================================
  Coverage   89.64%   89.65%           
=======================================
  Files          43       43           
  Lines        2240     2242    +2     
  Branches      248      249    +1     
=======================================
+ Hits         2008     2010    +2     
  Misses        159      159           
  Partials       73       73           

Impacted Files	Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	92.43% <100.00%> (+0.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a137bc2...4990a10. Read the comment docs.

[Oberon00]

Seems an expensive defense 😃

[c24t]

Seems an expensive defense 😃

I don't have a dog in this fight, just trying to close some old PRs. I'm happy to require immutable sequence attribute args instead or agree that the cost isn't worth the protection here.

[ocelotl]

Seems an expensive defense smiley

I don't have a dog in this fight, just trying to close some old PRs. I'm happy to require immutable sequence attribute args instead or agree that the cost isn't worth the protection here.

If this is of any help, we can also declare a class that implements an immutable sequence:

class ImmutableSequence:
    def __init__(self, sequence):
        self._sequence = sequence
    def __getitem__(self, index):
        return self._sequence[index]

Then, instead of taking a sequence and casting it to tuple (which is the slow part), we can just use it to instantiate an ImmutableSequence object which can't be mutated. Direct mutations to the original sequence will still mutate our object, though 🤷‍♂️ .

[Oberon00]

Direct mutations to the original sequence will still mutate our object

I think this is the main problem we want to protect against here though.

[Oberon00]

@c24t

I don't have a dog in this fight

So does anyone have an opinion on this PR?

[c24t]

So does anyone have an opinion on this PR?

This was @toumorokoshi's issue from #348 and #352. I'd prefer to merge this and revisit later if this ends up being a bottleneck. #352 (comment) suggests it won't be, and FWIW users can still call set_attribute with an immutable sequence after this change to avoid the copy.

[toumorokoshi]

I'd prefer to have this check in here now, and prevent as many erroneous uses as possible. In the future we can potentially remove this.

We already have validation of the span attributes in the repo as an additional defense. I guess I'm of the opinion of making this interface as defensive as possible.

we should probably have benchmarks to bring here. This isn't the most time-sensitive PR, so I'll try to bring that here.

Although we should also have a clearer idea of what we actually want our target overhead to be. otherwise the argument is too qualitative.

[mauriciovasquezbernal]

I don't have a strong opinion here, I'm slightly leaned toward having it, if we find it is very expensive during a follow up optimization we can remove it.

The thing that worries me now is that we have to guarantee that this validation is uniform across all places where attributes are used. For instance, now attributes validation is only done on Span.set_attribute, but nothing is done when creating an Event or a Link.

[toumorokoshi]

Thanks for the reimplementation!

[mauriciovasquezbernal]

Better to have this check for now. If during an optimization pass we find it is to expensive we can remove it.