Eliminate tag.Map API #59
Comments
|
I'd like to pick up that task. The question I have is what was the reason for having From what I have understood from the comment is that the current implementation of the map is not thread-safe and it is not feasible to require thread-safety from the vendor implementations. Is this right? If being able to provide an different tag.Map implementation by some vendor is still important then maybe it would be possible to achieve the thread safety by something like this: type Map interface {
Apply(update MapUpdate) Map
…
}
type lockedMap struct {
impl Map
implLock sync.Mutex
}
var _ Map = &lockedMap{}
func (t &lockedMap) Apply(update MapUpdate) Map {
t.implLock.Lock()
defer t.implLock.Unlock()
return t.impl.Apply(update)
}
// same for the rest of the Map interface methods
…
func NewEmptyMap() Map {
return &lockedMap{
impl: tagMap{}
}
}So in the end, vendor would need to provide an implementation of the |
This adds a lockedMap type which implements tag.Map interface. The implementation contains tagMap instance and a mutex, so all the operations are forwarded to the tagMap under the locked mutex. An additional care was needed for the functions returning contents of the map, because core.Value contains a byte slice, which has pointer like semantics. So to avoid accidental changes, we copy the value if it is of BYTES type. This likely should be handled by the core.Value itself, e.g. through some Copy function. The downside of locking here is that the users of Foreach function need to be careful to not call into the same map, otherwise deadlock will happen. While writing this code I think I have got an understanding of the issue at hand - the implementation of the tag.Map should basically be immutable (so the modification of the map actually produces a new map, instead of doing in-place updates, thus making the map threadsafe), but that is not easy (or even possible) to enforce. So maybe it's indeed better to avoid providing the ability of having a different, vendor-specific implementation of tag.Map and have a good-by-default implemetation as a part of API. Still, to preserve the immutability of the map, the core.Value structs need to be deep-copied. Fixes open-telemetry#59
|
I'm not sure there was any reason to have a tag.Map interface in the first place. As I think we've discovered in #52 (see also open-telemetry/oteps#11), there will be uses of the tag.Map interface that are expected before the SDK loads. So, I think we should eliminate the interface and use a concrete type. Then, having an immutable map is sufficient to avoid the problems with map concurrency. I believe the point about deep-copying bytes is also true, but I want to point out some related work. I have an as-yet-unwritten proposal about supporting structured values and lazy values, for various reasons covered in open-telemetry/opentelemetry-specification#76. The code (in Go) is a proof-of-concept: jmacd#3 |
This is to make tag.Map an immutable type, so it is safe to use concurrently. The safety is not yet fully achieved because of the functions returning contents of the map (Value and Foreach). The functions give callers an access to core.Value objects, which contain a byte slice, which has pointer like semantics. So to avoid accidental changes, we will need to copy the value if it is of BYTES type. Fixes open-telemetry#59
This is to make tag.Map an immutable type, so it is safe to use concurrently. The safety is not yet fully achieved because of the functions returning contents of the map (Value and Foreach). The functions give callers an access to core.Value objects, which contain a byte slice, which has pointer like semantics. So to avoid accidental changes, we will need to copy the value if it is of BYTES type. Fixes open-telemetry#59
This is to make tag.Map an immutable type, so it is safe to use concurrently. The safety is not yet fully achieved because of the functions returning contents of the map (Value and Foreach). The functions give callers an access to core.Value objects, which contain a byte slice, which has pointer like semantics. So to avoid accidental changes, we will need to copy the value if it is of BYTES type. Fixes #59
The Map API is unsafe, as explained by @cstockton in #23 (comment)
Make it a concrete type.
The text was updated successfully, but these errors were encountered: