fix: Update attribute type conformance validations to be in line with current OpenTelemetry Semantic Conventions

[0robustus1]

OpenTelemetry v1.51.0 introduced a wider definition of AnyValue. This PR aims to validate attribute values against that newer spec rather than the older one.

[xuan-cao-swi]

Thanks for updating this to comply latest spec.
Have you considered using a recursive approach instead?

def valid_value?(value, depth = 0)
  # ...
  case value
  when String, TrueClass, FalseClass, Integer, Float, NilClass
    true
  when Array
    value.all? { |v| valid_value?(v, depth + 1) }
  when Hash
    value.all? { |k, v| valid_key?(k) && valid_value?(v, depth + 1) }
  else
    false
  end
  # ...
end

[0robustus1]

Yes, I originally used the recursive approach, but I was a bit concerned about larger more complex JSON style attributes, since there isn't a natural limit on nesting.

[mwear]

Thanks for putting this together. I don't think we can move forward with this at this stage, for the reasons below:

The spec did widen AnyValue to include complex values, but in v1.51.0 those additions are all marked Status: Development, not stable. Only the primitives and homogeneous primitive arrays are stable. So this is really adopting development-stability spec features as the default validation behavior, and I don't think we should do that without a way to turn it off.

It also isn't contained to the validator. Anything that consumes attributes has to understand the wider shape, and our exporters don't yet. Concretely: as_otlp_any_value in the OTLP exporter has no Hash branch, so a map attribute passes validation, produces no error, then serializes to an empty AnyValue and disappears on export. truncate_attribute_value doesn't descend into hashes either. As it stands, the change lets people set values that silently vanish downstream.

Given the development-level stability, this needs to be opt-in behind a flag, with the default staying on the current stable behavior. No other SDK accepts maps for standard attributes by default as far as I know.

Separately, I don't think the set-based cycle tracking is correct. seen is never unwound, so it flags any object that appears more than once, not just an ancestor on the current path. A shared but acyclic value gets falsely rejected. For example shared = [1, 2]; span.set_attribute('foo', [shared, shared]) is valid, but comes back invalid. Cycle detection needs to track the current path, not everything visited.

Happy to talk through what an opt-in version would look like if you want to take it that direction.

[mwear]

I also wanted to note that there is a proposal to limit the depth for array and map attributes. I think that would make the recursive solution the most straightforward and preferred option.

[0robustus1]

@mwear, the AnyValue extension is no longer marked as Development since v1.53.0 (with v1.58.0 being current).

So in general I would say that it is valid and encouraged for SDKs to develop against that spec, especially when the spec allows something that the SDK currently does not.

I'll take a closer look at the other changes that would be necessary to not drop these attributes.

Regarding the depth proposal: That is only a proposal at this stage, isn't it?

Regarding the seen handling: Yes, this would treat the same object (Array, Hash) repeated as siblings the same, which isn't a cycle. I'll take a look at addressing that as well.

[mwear]

Sorry for the confusion about stability. You're right that it's been stable since 1.53.0. I followed the link in the description and saw development without comparing to current spec (my bad). We don't need this to be gated by a flag.

The depth proposal is only a proposal at this stage, however, there's no disagreement that limits on array and map values are needed. The only debate is what shape that should take. Given that we limit all other attribute types I think we can safely assume there will be a limit for maps and arrays.

One last thing that I would like to point out is that attribute validation is a hot code path. We're very sensitive to any unnecessary object allocations on hot paths. One unfortunate reality about the Set based approach is the Set allocation per validation. For this reason, and the likely need to track depth make the recursive solution more attractive.

[0robustus1]

One last thing that I would like to point out is that attribute validation is a hot code path. We're very sensitive to any unnecessary object allocations on hot paths. One unfortunate reality about the Set based approach is the Set allocation per validation. For this reason, and the likely need to track depth make the recursive solution more attractive.

That's a good point. I had considered the possibility of StackLevelToDeep errors as more problematic, but you are right, the Set negatively impacts all validations, while the SystemStackError scenarios would really only hit the problematic cases. I'll adjust it back to recursion and ensure that we handle the SystemStackError issue gracefully.