ddtrace/tracer: add integration tag to spans_started/finished #3023
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 5132 Passed, 70 Skipped, 2m 34.62s Total Time |
BenchmarksBenchmark execution time: 2025-01-10 22:39:38 Comparing candidate commit ce1b7eb in PR branch Found 0 performance improvements and 8 performance regressions! Performance is the same for 50 metrics, 1 unstable metrics. scenario:BenchmarkOTelApiWithCustomTags/otel_api-24
scenario:BenchmarkPartialFlushing/Disabled-24
scenario:BenchmarkPartialFlushing/Enabled-24
scenario:BenchmarkSetTagMetric-24
scenario:BenchmarkSingleSpanRetention/no-rules-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24
scenario:BenchmarkTracerAddSpans-24
|
hannahkm
changed the title
github-actions
bot
added
the
apm:ecosystem
There was a problem hiding this comment.
Ok, so you're reporting spansStarted/spansFinished on span.Start/span.Finished if the integration is not empty, and leaving the chunk reporting to any spans that are manual... I understand why you did this but not totally sure about the approach.
span.Start and span.Stop are typically called quite frequently, so if a majority of the spans are from automatic integrations, this will be very noisy (and defeats the purpose of reporting the metrics at a specified interval, to reduce noise)
One alternative idea:
Change the way we track spansStarted and spansFinished to be some kind of counter map that includes the integration name, e.g. map[string]uint32 where the key is the integration name and the value is the count of spans started/finished that integration name. Then, in this goroutine, we'll have to iterate over the map and report the spans started/finished per integration
(or some other idea I haven't thought of?)
|
@mtoffl01 Good points! A map would probably work better; I was hesitant at first since I didn't want to change too much of what already exists, but knowing that these metrics are pretty old... I'm more down to change it up now. |
|
@hannahkm I'm approving this but we should investigate why the benchmarks report the increased allocations. |
There was a problem hiding this comment.
Overall, I definitely have some concerns 🤔 Maybe you can write some additional tests to provide peace of mind....
- Tests designed to try and make the system fail -- what happens when you have multiple goroutines access a start span / finish span method, can we prove that we've protected against a race condition?
- Maybe you want to write dedicated benchmarks to show how much performance is impacted
ddtrace/tracer/metrics.go
Outdated
| t.spansStarted.mu.Lock() | ||
| for name, v := range t.spansStarted.spans { | ||
| t.statsd.Count("datadog.tracer.spans_started", int64(v), []string{"integration:" + name}, 1) | ||
| t.spansStarted.spans[name] = 0 |
There was a problem hiding this comment.
Interesting. At first I was going to suggest that you use delete instead, since delete is generally more performant (constant time complexity and removes the key from the map, freeing up memory), but in this case, it's possible and even likely that the same key will be reused in the map in the future. In this case, it's possible that setting the value to 0 will be more performant, since you won't need to re-allocate space for the key in the map. I'm curious if you chose to set the value to 0, rather than use delete, for this purpose?
There was a problem hiding this comment.
Yeah, I assumed that it would be more likely for a key to be reused than to be a one off and done. But I do wonder if we have data about which situation is more common (i.e. many spans being created for a certain integration vs just a few, all at once).
ddtrace/tracer/metrics.go
Outdated
| t.statsd.Count("datadog.tracer.spans_finished", int64(atomic.SwapUint32(&t.spansFinished, 0)), nil, 1) | ||
| t.spansStarted.mu.Lock() | ||
| for name, v := range t.spansStarted.spans { | ||
| t.statsd.Count("datadog.tracer.spans_started", int64(v), []string{"integration:" + name}, 1) |
There was a problem hiding this comment.
I think you may want to use Incr here instead.
There was a problem hiding this comment.
Hm, why use Incr? If there has been more than one span started from a certain integration, the value of v should be greater than 1, so we would want to use Count.
ddtrace/tracer/metrics.go
Outdated
| t.spansStarted.mu.Unlock() | ||
| t.spansFinished.mu.Lock() | ||
| for name, v := range t.spansFinished.spans { | ||
| t.statsd.Count("datadog.tracer.spans_finished", int64(v), []string{"integration:" + name}, 1) |
There was a problem hiding this comment.
Same as line 98: Use Incr.
Co-authored-by: Mikayla Toffler <[email protected]>
ddtrace/tracer/tracer.go
Outdated
| spansStarted, spansFinished, tracesDropped uint32 | ||
| // These maps keep count of the number of spans started and finished from | ||
| // each component, including contribs and "manual" spans. | ||
| spansStarted, spansFinished *xsync.MapOf[string, int64] |
There was a problem hiding this comment.
To fix the perf issue you're seeing, try using a *atomic.Int64 type instead of int64 for the map value. Also use the Load+LoadOrStore method on the map to do the update like shown in the example I posted earlier.
After the initial phase of initializing the map keys, the code will end up taking only the Load path which is much faster than Compute. The latter always needs to take a lock whereas the former uses optimistic concurrency control that requires no locks or retries during steady state.
Let me know if that works.
(I know you mentioned you tried Load+Store in your slack DM, but I suspect you didn't try with an *atomic.Int64 pointer like I'm suggesting?)
| if !ok { | ||
| v, _ = t.spansStarted.LoadOrStore(span.integration, new(atomic.Int64)) | ||
| } | ||
| v.Add(1) |
There was a problem hiding this comment.
Just confirming that at this point, v is of type *atomic.Int64, and not interface{}?
| // reset the count | ||
| t.spansStarted.Range(func(key string, value *atomic.Int64) bool { | ||
| err := t.statsd.Count("datadog.tracer.spans_started", value.Load(), []string{"integration:" + key}, 1) | ||
| t.spansStarted.Delete(key) |
There was a problem hiding this comment.
I wrote a comment about this on a previous commit. I wonder if you want to Delete here which [if syncx.MapOf follows golang std map functionality] will deallocate the data at this key, or if you want to just set the value at key to 0.
As I understand it: Delete is "greedy" and good if we want to get rid of memory allocation as soon as possible, but could be bad if we frequently allocate data at the same key over, and over again.
Setting the value to 0 is bad if we won't re-use this key in the future (because we're letting unused memory sit there), but good if we frequently re-use it.
Given that our tracer has a set number of integrations it supports, I imagine we will re-use the same key quite often, and would therefore lean toward the second option (keep key, set to 0).
What does this PR do?
Add a
integrationtag to the existingdatadog.tracer.spans_startedanddatadog.tracer.spans_finishedmetrics. The value of the tag will be the name of the component from which the span was started. For example, for a contrib, it will be the name of the contrib package (chi,net/http, etc). For spans that were created manually, the tag will saymanual.Motivation
We want to know, in addition to when a span is started, where the span originated from. This could be a contrib or a manual implementation.
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!