ddtrace/tracer: force trace stats flush on Stop #1393
Conversation
Kyle-Verhoog
force-pushed
the
kylev/force-stats
branch
from
4526453
to
73a1faf
Compare
Kyle-Verhoog
force-pushed
the
kylev/force-stats
branch
from
d8d6f75
to
a5d5d98
Compare
The trace stats were not being sent on tracer.Stop() / tracer.stats.Stop() so any unsent buckets would never be sent. The fix is to add an option to force the bucket out on stop.
Leave flush as-is so that the lock acquisition doesn't cover the transport call.
Kyle-Verhoog
force-pushed
the
kylev/force-stats
branch
from
9e5440e
to
3d07468
Compare
waitForBuckets was not releasing the lock when the function would return true... and then the transport needed to be specified to avoid a nil pointer dereference... Thank you Andrew!!
ajgajg1134
force-pushed
the
kylev/force-stats
branch
from
7b32635
to
47a3abf
Compare
ddtrace/tracer/stats.go
Outdated
| bucketSize int64 // the size of a bucket in nanoseconds | ||
| stop chan struct{} // closing this channel triggers shutdown | ||
| cfg *config // tracer startup configuration | ||
| wg *sync.WaitGroup // waits for any active goroutines |
There was a problem hiding this comment.
Please don't make a pointer out of this. Waitgroups are generally not declared as such.
There was a problem hiding this comment.
Interesting, I've always used the opposite approach to make accidentally copying the waitgroup more difficult, what's the reasoning behind keeping it as not a pointer?
There was a problem hiding this comment.
I think the question goes the other way: what's the reason to have it as a pointer? You never have to initialise it, it's ready-to-use. How often does it happen that you need to pass it around? And if you do need to pass it around, it can always be dereferenced.
I frankly have never seen it passed as a pointer in official Go projects (e.g. the lang), unless it's an argument to a function that is supposed to receive a copy.
I don't think that declaring things as pointers to make sure we don't accidentally copy them is a good enough reason to IMHO.
There was a problem hiding this comment.
My thought process was that all the methods on wait group have a pointer receiver so using it as a pointer makes it slightly more obvious that's the typical way to use it. In this specific spot I changed it to a pointer as part of debugging to make sure we weren't accidentally copying it since the tests were failing in an odd way. I'll switch this back to non-pointer since it's not really bringing a ton of value (and if we do accidentally copy it the unit tests will protect us)
There was a problem hiding this comment.
Please ping me if you do discover that having it as a pointer provides some kind of benefit or that not having it has caused some sort of issue. Currently, it just seems a lot easier to use because it's ready-to-use without the need for initialising.
There is no reason to use either of these functions individually so makes sense to merge them.
The trace stats were not being sent on tracer.Stop() /
tracer.stats.Stop() so any unsent buckets would never be sent.
The fix is to add an option to force the bucket out on stop.