ddtrace/tracer: add _dd.base_service tag#2175
Conversation
BenchmarksBenchmark execution time: 2023-09-05 12:14:18 Comparing candidate commit 735d692 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics. |
073fa48 to
71587a7
Compare
rarguelloF
changed the title
ahmed-mez
left a comment
ahmed-mez
left a comment
There was a problem hiding this comment.
BenchmarkPartialFlushing is the most impacted benchmark likely because it calls genBigTraces which uses a custom service name.
Mind pushing a commit with genBigTraces without a the ServiceName("pylons") just to confirm it?
| spans := run(t, nil, spanOpts) | ||
| for _, s := range spans { | ||
| assert.Equal(t, "span-service", s.Service) | ||
| assert.NotEmpty(t, s.Meta["_dd.base_service"]) |
There was a problem hiding this comment.
What's the expected value here? Can we use assert.Equal?
There was a problem hiding this comment.
This is the fallback default service name value, that we obtain calling filepath.Base(os.Args[0]). I didn't want to assert.Equal to any concrete value because it might change depending on how you run the tests.
I guess an option would be to assert.Equal(t, filepath.Base(os.Args[0]), s.Meta["_dd.base_service"]), but then the test would fail in case we decide to change this logic some day. What do you think?
There was a problem hiding this comment.
Thanks for explaining, I don't think we should leak the implementation details of the defaulting to this unit test. So you can keep the code as is and just add a comment to say why we are not asserting with Equal.
There was a problem hiding this comment.
LGTM. I'm fine with the changes in genBigTraces since it's used for partial flushing benchmarking and not having a custom service per span will not impact partial flushing per se.
I couldn't find a way to improve the perf either, the logic is pretty straightforward and the 3% perf regression is acceptable in this case I believe.
@rarguelloF one final question, did we have any figures on how often this scenario happens in real life? (i.e overriding DD_SERVICE)
2 participants
What does this PR do?
Adds
_dd.base_servicetag, which is set only for spans that set a service name different than the tracer-level globally configured service name.Motivation
The tag is used internally to improve service map.
Reviewer's Checklist