Add() is not thread-safe when called with more than one attribute #3943
Comments
|
Thanks, @jmacd, for helping me to point out the source (I'm a newbie in Go): We have the call to NewSet looks like this: So it means the user-provided array is sorted for them, without them knowing it. It means that they will not put a lock on it, as they will not expect it, hence when this array is re-used (it should since you keep recording values) across multiple Go Routines using different native threads, it will cause a race condition as presented, hence a bug. |
|
Hi, Can I take this bug? |
|
@Kaushal28 I assigned you |
|
The is that you construct the |
|
Thanks @asafm @pellared. Could you help me reproduce? When you say func main() {
provider := metric.NewMeterProvider()
meter := provider.Meter("test_meter")
counter, err := meter.Int64Counter("int64counter")
if err != nil {
panic(err)
}
ctx := context.Background()
attrs := []attribute.KeyValue{
{
Key: "key1",
Value: attribute.StringValue("val1"),
},
{
Key: "key2",
Value: attribute.StringValue("val2"),
},
}
go func() {
for i := 0; i < 1000; i++ {
counter.Add(ctx, 1, attrs...)
}
}()
go func() {
for i := 0; i < 1000; i++ {
counter.Add(ctx, 1, attrs...)
}
}()
} |
|
I'm not sure this is a problem that can be solved in the SDK outside of copying the passed attributes before making a set out of them. This still provides a potential for a data race during the copy. It might be the case that the API needs to be updated to accept attribute sets instead. Give the immutability of attribute sets this will allow them to be reused concurrently. One draw back is the API will lose the nice variadic attribute design. For example, |
|
@MrAlias Yes please :) . Taking an immutable set will also avoid allocating and copying the attributes each time we need to record an event. |
|
|
In my opinion, the current API is working as designed. Comments on the synchronous instruments should state that the input slice of By the way, this conversation is related to open-telemetry/opentelemetry-collector#7455 where I've stated that even calling I think it should be possible to work around the race condition by pre-sorting the list of attributes. For example, I expect I think it would be appealing to support alternative APIs with both variations, meaning for every |
|
@jmacd What if |
It does not seem appropriate to solve our race condition by saying a user needs to pass us "cleaned" values of an argument. Especially if doing so would have them create a set anyway. Asking users to clean their input also does not solve the underlying issue, the API would not be concurrent safe under certain situations. This is non-compliant with the specification.
This supports the change to the API, right? Currently for all our synchronous measurement operations we create a set.
Give we are still pre-1.0 for this API I prefer to not add methods to fix an existing method. Rather we should just fix the existing method. |
|
@MrAlias I'm worried about loss of performance for existing use-cases that are correct as they stand. We can work around this issue by copying the input slice for every caller, but only special callers where the compiler does not insert a temporary slice need this support.
I think requiring an attribute.Set burdens the user with an inconvenient API. My preference would be to document that users must either use a compiler-generated slice or must take their own lock. Also, I believe the optimizations in the Lightstep SDK correctly work around this race (when
On the other hand, when I set the I think I've demonstrated how the race can be fixed w/o changing our API here--this is also discussed in open-telemetry/opentelemetry-collector#7455. |
In a way, yes. Although I find it unergonomic, changing the API to accept If a caller is currently repeating attribute sets but has no good way to cache the result of calculating an The original OTel conversation about "bound instruments" was another angle on this performance problem. If the user is able to store and cache something to benefit performance, that's nice but it could force them into a major code reorganization. This is where I get the idea that two APIs would be great -- the convenient API and the fast API. Even if we have a way to pass |
I think that's what's really happening right now, no? You have: myCounter.Add(context.Background(), 1, attribute.String("x", "y"), attribute.Int("z", 2))The |
How is copying data and then creating a set more performant than just accepting the set?
The set construction does not require an allocation unless it is the only set created. The set temporary pointer is pooled. |
Sure, but is it less performant than it is more performant for a user to just re-use a set instead of allocating a new group of attribute args each time? It seems like there is an order of magnitude between the two and accepting a set allows the user to choose how performant they want to be or not without us having to add an additional bound instrument API |
|
Another reason to accept a set instead of a slice of attributes is that that is what is actually used. There are syntactical niceties to accepting variadic Accepting a set makes it clear to the user that duplicate keys are not allowed. |
Do you have benchmark numbers on this? I'd be interested to see the difference. For what it is worth, I have, in a local branch, extended the |
|
Another suggestion from the SIG meeting today (cc @MadVikingGod): Have methods that accept no attributes and an equivalent type SyncAdder interface {
Add(ctx context.Context, value N)
AddWithAttributes(ctx context.Context, value N, attrs attribute.Set)
}
type SyncRecorder interface {
Record(ctx context.Context, value N)
RecordWithAttributes(ctx context.Context, value N, attrs attribute.Set)
}
type AsyncObserver interface {
Observe(value N)
ObserveWithAttributes(value N, attrs attributes.Set)
}This would help eliminate the boilerplate of calling It's not immediately clear if this will comply with the specification though, i.e.:
If the specification is talking about a logical API, I think having multiple methods to satisfy it is compliant. |
PoC: #3955 This approach works well for API users that do not pass attributes. For example, this: counter.Add(ctx, 100, *attribute.EmptySet())Becomes: counter.Add(ctx, 100)However, when a user includes attributes, there is a noticeable number of characters added to the line. For example, the original: counter.Add(ctx, 100, attrs)Becomes: counter.AddWithAttributes(ctx, 100, attrs)It seems that this proposal has the trade-off of minimizing boilerplate for the no-attribute case and adding boilerplate in the attribute case. Quantitatively |
|
A user could always reduce their boilerplate by declaring counter.Add(ctx, 100, *attribute.EmptySet())
hist.Record(ctx, 10, *attribute.EmptySet())into: noAttr := *attribute.EmptySet()
counter.Add(ctx, 100, noAttr)
hist.Record(ctx, 10, noAttr)Conversely, trying to reduce the boilerplate of var ctrAdd = counter.AddWithAttributes
ctrAdd(ctx, 100, attr)
var histRec = hist.RecordWithAttributes
histRec(ctx, 100, attr)would have limited functionality as it would only apply to one instrument. |
|
The concern I have is less about the boilerplate, and more that we want to encourage people to use a particular empty set ( |
After #3957 they will be able to pass We could comment |
I think, I would prefer to have a single method. It reduces the API surface which IMO has benefits for both users of the API and for the developers of API implementations. Users who do not like boilerplate can always create their own helpers/adapters. |
|
I'm still concerned about an additional, obligatory memory allocation that results from having a single API accept the current The user in this case already has organized their code to produce a list of attributes, as typical of a statsd library. If we replace the three instrument operations w/ a shared attribute.Set, that is an improvement in a sense, because if the SDK will compute the attribute.Set each time then letting the user compute it once is an improvement. In the case of the Lightstep metrics SDK, I mentioned the use of a This leads me to think that we should find a way to eliminate attribute.Set in favor of something better, call it the I guess this leads me to agree that it would be nice for high-performance applications to have an option to amortize the construction cost of attribute sets; it's also possible for SDKs to avoid the attribute.Set allocation itself. Can we have the best of both? |
|
Has one checked how other language's API are handling this problem? I did a quick analysis.
However
So all Java, Python, Node.js accepts an attributes object that CAN be concurrent safe. Everything depends on the SDK and the caller.
As far as I understand this is what Java, Python and Node.js are doing 😬 However, right now in Go API we cannot even pass a concurrent-safe attribute slice. Personally, I think that @MrAlias proposal #3947 is very clean and pragmatic.
EDIT: I think @MrAlias is the way to go. How the API user would not which Set implementation should be used? |
@pellared Thank you for the research. In a way, it looks like other repositories have validated my first position in this debate which is that it's WAI--the user shouldn't expect the Ultimately, this debate is about the performance hit to a specific kind of user. I agree that #3947 is clean and pragmatic, but I believe it's going to cause a performance hit to a different kind of user--an obligatory allocation happens to construct the set if we don't redesign the I was able to correct the race condition in the Lightstep metrics SDK such that no attribute.Set is constructed when an entry already exists in the map.. This shows that it is possible to work around the race condition without changing our API or sacrificing performance. I'm interested in the outcome of this discussion because I want to make sure both kinds of user are optimized for, and then I want even more optimization: if the attribute.Set could be shared across call sites and I could keep my fingerprinting optimization, that would be ideal. I believe we could arrange a new API such that would support repeated use of the attributes set and allow the SDK to avoid allocating the attribute set when it already has the same in memory. I'm afraid the thing we'll lose is the ability to write a one-line metrics update with attributes passed as varargs at the call site. I see this as a minor loss and if everyone else is OK with it, that's fine. |
@jmacd, do we have any benchmark or reported issue which we can use as a reference? I do not see any problems when I see these benchmark results. |
Why can't this pooling approach not be used with the func (thing *Thing) instrumentOperation(ctx context.Context, value1, value2, value3 float64, moreAttrs ...attribute.KeyValue) {
attrs := attrsPool.Get().(*[]attribute.KeyValue)
// get some attributes from the "scope" (thing.myAttributes...)
// get some attributes from the context (e.g., from baggage...)
// get the attributes from the call site, if any (i.e., moreAttrs...)
// the slice of attributes is assembled
s := attribute.NewSet(*attrs...)
instrument1.Add(ctx, value1, s)
instrument2.Add(ctx, value2, s)
instrument3.Record(ctx, value3, s)
attrsPool.Put(attrs)
}This seems like it would still cut down allocations given var attrsPool = sync.Pool{
New: func() any {
p := new([]attribute.KeyValue)
*p = make([]attribute.KeyValue, 0, 16)
return p
},
}
type Adder struct {
name string
w io.Writer
}
func (a *Adder) AddAttr(v int64, attr ...attribute.KeyValue) {
cp := make([]attribute.KeyValue, len(attr))
copy(cp, attr)
fmt.Fprintf(a.w, "%q.AddAttr(%d, %v)\n", a.name, v, cp)
}
func (a *Adder) AddSet(v int64, s attribute.Set) {
fmt.Fprintf(a.w, "%q.AddSet(%d, %v)\n", a.name, v, s)
}
type Inst struct {
attr []attribute.KeyValue
adder0 *Adder
adder1 *Adder
}
func NewInst(w io.Writer, attr ...attribute.KeyValue) *Inst {
return &Inst{
attr: attr,
adder0: &Adder{name: "adder0", w: w},
adder1: &Adder{name: "adder1", w: w},
}
}
func (i *Inst) AddAttr(v0, v1 int64, moreAttrs ...attribute.KeyValue) {
attrs := attrsPool.Get().(*[]attribute.KeyValue)
*attrs = (*attrs)[:0]
*attrs = append(*attrs, i.attr...)
*attrs = append(*attrs, moreAttrs...)
i.adder0.AddAttr(v0, *attrs...)
*attrs = append(*attrs, attribute.Bool("additional", true))
i.adder1.AddAttr(v1, *attrs...)
attrsPool.Put(attrs)
}
func (i *Inst) AddSet(v0, v1 int64, moreAttrs ...attribute.KeyValue) {
attrs := attrsPool.Get().(*[]attribute.KeyValue)
*attrs = (*attrs)[:0]
*attrs = append(*attrs, i.attr...)
*attrs = append(*attrs, moreAttrs...)
s := attribute.NewSet(*attrs...)
i.adder0.AddSet(v0, s)
*attrs = append(*attrs, attribute.Bool("additional", true))
s = attribute.NewSet(*attrs...)
i.adder1.AddSet(v1, s)
attrsPool.Put(attrs)
}
func BenchmarkAddAttr(b *testing.B) {
i := NewInst(io.Discard, attribute.String("user", "alice"))
b.ReportAllocs()
b.ResetTimer()
for n := 0; n < b.N; n++ {
i.AddAttr(1, 2, attribute.Bool("admin", true))
}
}
func BenchmarkAddSet(b *testing.B) {
i := NewInst(io.Discard, attribute.String("user", "alice"))
b.ReportAllocs()
b.ResetTimer()
for n := 0; n < b.N; n++ {
i.AddSet(1, 2, attribute.Bool("admin", true))
}
} |
|
|
This specification issue is looking to add an additional argument to That assumption appears to have been incorrect. I'll plan to look into a proposal that will take that approach. |
I think there are still a possible race conditions in the PR. Doing From the PR: // We have to copy the attributes here because the caller may
// modify their copy of the list after the call returns.So the attributes can be modified "during" the call by a separate goroutine. References:
|
I don't think so. The original caller is still active on the call stack, so if the slice is modified at this point it means the slice was modified by the caller during its own call. |
|
After reading these proposals, I'm left with the impression that supporting two separate APIs would be the least complicated solution. I'm so interested in making sure that callers have a way to avoid all allocations at their call site that I'll take any of the style proposals we can work out.
I think I disagree, unless we can completely change the attribute.Set type. That type is not a reusable object, so making a pool of them won't help us, I think? Actually, a lot of the dialog above rests on the assumption that |
Right, agreed, the proposal would be a pool for |
|
I'm not worried about allocation of I'm worried about the allocation of the |
I think we are talking past each other, my statements are in regard to the "Add |
@jmacd can you explain this? I'm still not following how accepting a It seems like in your distro you have tried to optimize the use of |
|
Description
.Add()is not thread-safe when used with more than one attributeEnvironment
Steps To Reproduce
.Add(()with 2 attributes, from multiple Go routines.Expected behavior
It should work without errors.
Here's the race detector stack race:
The text was updated successfully, but these errors were encountered: