Span attribute control implementation is incoherent
garthk opened this issue · 3 comments
To be compatible with the OpenCensus protobuf protocol, an attribute value MUST be one of:
TruncatableString
int64
bool_value
double_value
Strikes me libraries for creating spans SHOULD prevent creation of attributes that OpenCensus Collector and OpenCensus Reporter can't handle, as otherwise end users will run into trouble whenever they change their ops infrastructure.
Some destinations are even stricter, e.g. Datadog (opencensus-beam/opencensus_datadog#8). For the same operational flexibility reasons, their export libraries SHOULD cast attributes their target can't handle to strings.
You might well argue that, no:
- Each language has its own ecosystem of libraries
- The supported attributes are up to them
- The definition of AttributeValue is relevant only to the Go ecosystem, despite it being used in some software we encourage end users to deploy
Whatever angle you take, though, there's something about our version 0.9.1 that's wrong.
Despite its type declarations, :ocp.put_attribute/2
in 0.9.1 is even stricter than AttributeValue:
iex> :ocp.with_child_span("name")
iex> :ocp.put_attribute("double", 1.0)
{:error, :invalid_attribute}
None of the other ways to put attributes restrict attribute values, including:
:oc_span.put_attribute/3
,:oc_span.put_attributes/2
,:oc_trace.put_attribute/3
,:oc_trace.put_attributes/2
,:ocp.put_attributes/1
I think we have to do a drop in the reporter. I forgot about the fact that attribute value can be a function to be lazily created, so we won't know the type at time of creation, only in the reporter.
We should probably provide a utils module that has functions to help with this stuff instead of being duplicated in all reporters.
Fair enough. I'll rework the attribute processing in the PR for opencensus_elixir
accordingly. Do the functions get called by :oc_reporter
before being passed to the configured reporter behaviour? Can we fix the typings?
Right now they are called by each reporter and not by oc_reporter
. It is tough to say which is better.. since many reporters have limits and will drop attributes, so in those cases the attribute value would never be resolved at all.
And yea, we need to fix the types.