Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Span attribute control implementation is incoherent #156

Open
garthk opened this issue Jul 3, 2019 · 3 comments
Open

Span attribute control implementation is incoherent #156

garthk opened this issue Jul 3, 2019 · 3 comments

Comments

@garthk
Copy link

garthk commented Jul 3, 2019

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
@tsloughter
Copy link
Member

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.

@garthk
Copy link
Author

garthk commented Jul 12, 2019

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?

@tsloughter
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants