Improve log correlation performance

[TonyCTHsu]

Inspired by #3447

What does this PR do?

This is a performance improvement for correlation object.

1. Creating correlation object from trace operation (skipping TraceDigest)
2. Improve version, env and service without duplicate in memory

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-03-05 17:12:04

Comparing candidate commit b15be92 in PR branch tonycthsu/fast-correlation with baseline commit 4f00746 in branch 2.0.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 2 unstable metrics.

scenario:Tracing.log_correlation

• 🟩 throughput [+56503.683op/s; +56753.555op/s] or [+84.062%; +84.434%]

[delner]

Just trying to make sure I have a complete understanding of the change. I think the broad strokes are TraceOperation --> TraceDigest --> Correlation::Identifier.

• TraceDigest is a full-copy of the the trace state basically, whose purpose is propagation or some other kind of snapshotting of the trace.
• Correlation::Identifier is a much smaller struct, whose purpose is to be printed into logs, or consumed by other processes (aka profiling or ASM)

The new to_correlation function is basically what identifier_from_digest was minus the need for a TraceDigest. That makes sense, in that it skips a lot of the allocation involved in building an immutable TraceDigest object. I'm on board with this.

Still, some questions in my mind were:

1. Do we lose data that would've been in the TraceDigest by skipping it?
   • Answer: I don't think so. It looks like there's no place in code that has ever populated service, env, version, or source on a Correlation::Identifier explicitly. But one could argue identifier_from_digest (from which this was copied) is coded incorrectly, and that it should've populated those values from the digest, not from Datadog.configuration. I think this is a bug, and probably should be corrected. This could be said to be severable from the new to_correlation. Still perhaps to_correlation should be populating those fields from the active_span or the TraceOperation instead. You may want to change this.
2. Is building a Correlation::Identifier directly in the Tracer a breach of encapsulation? Should it have to go through a function analogous to Correlation.identifier_from_digest?
   • Answer: I don't think so. Identifier is a simple struct with some helpers attached to it. I don't think it needs to be hidden behind a helper function.

My own read on the history of Correlation seems to suggest it started off as a really simplistic struct attached to Context way back in 0.19, but has snowballed a bit picking up additional fields out of convenience. Then we (by that I mean I) naively decided to rebase it upon TraceDigest because it seemed like a nice, logical, clean thing to do at the time. I was not considering lean, mean functions at the time.

Bottom line: I think the change is fine. It's certainly nice to see a speed up in throughput like that. Consider addressing the issue outlined in (2) though.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.08%. Comparing base (9ed085b) to head (145613f).

Additional details and impacted files

@@           Coverage Diff           @@
##              2.0    #3486   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files        1219     1219           
  Lines       71543    71589   +46     
  Branches     3404     3406    +2     
=======================================
+ Hits        70173    70221   +48     
+ Misses       1370     1368    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.