Remove Span.ModifyAttributes()

[open-telemetry]

Remove Span.ModifyAttributes()

[jmacd]

I created the prototype using OpenCensus for inspiration, so that's where this came from, and the initial prototype in this repository was a streaming implementation. These Modify operations are definitely compatible with a streaming model.

The reader of the stream is responsible for reconstructing Span attributes at each point in the stream. Modify operations are simply applied by the reader to update the attribute state at subsequent points in the stream.

This raises questions, like how is a user to un-set an attribute? How is a user to delete an attribute? How will the OpenCensus libraries handle this transition? @bogdandrutu ^^^ more of a spec question than a Golang question.

[yurishkuro]

@jmacd what is the use case for deleting / unsetting an attribute? Such functionality did not exist in OpenTracing, and nobody asked for it, iirc.

[jmacd]

The tag.Map code is shared by context tags, which do support deleting entries. Exposing the mutators via ModifyAttribute was speculative.

I feel that OpenTracing didn't specify this well enough. I imagine we could find uses of Set("error", false) following Set("error", true), made with the intention of unsetting an error attribute. For string-valued attributes, I wouldn't be surprised to see an empty string used to unset an attribute. Why wouldn't the program compute the value it intends and set it once? It doesn't feel too exotic to me: e.g., you call Set("error", true) on the fly as errors happen, but in a context handler somewhere near the top of your request you decide that canceled requests are never errors, so you unset it in that case.

I don't think this is a big deal, and I'm more than happy to remove ModifyAttribute from the Span API, as well as Upsert from the tag.Map.

[VineethReddy02]

Hey @jmacd I would like to work on this, I am new to tracing and would like to contribute to
open-telemetry,
So I notice
In span package and

func (sp *span) ModifyAttributes(mutators ...distributedcontext.Mutator) {
	sp.sdk.exporter.Record(exporter.Event{
		Type:     exporter.MODIFY_ATTR,
		Scope:    sp.ScopeID(),
		Mutators: mutators,
	})
}

In trace package

// ModifyAttributes does nothing.
func (s *span) ModifyAttributes(mutators ...apidctx.Mutator) {
}

but I don't see any method definition in trace package it's an empty function. I see this in multiple places.

So the scope of this issue is to remove it from all the places?

[jmacd]

Yes. In the prototype, this pattern of supporting Mutators was carried over from the OpenCensus code base. We'll remove it, which means any Mutator should disappear. All ModifyAttributes disappear, only SetAttribute functionality remains in this respect. The MapUpdate type should have only two fields, supporting an update for multiple Set operations or a single Set operation.