JaegerExporter GenerateServiceSpecificBatches option

[CodeBlanch]

Fixes #1388.

Public API Changes

Makes the JaegerExporter batch behavior configurable by adding a new option:

        /// <summary>
        /// Gets or sets a value indicating whether or not a batch should be sent to the Jaeger agent for each service. Default value: true.
        /// </summary>
        /// <remarks>
        /// Jaeger UI will only detect &amp; color dependency spans when both
        /// the client &amp; server processes report data to the same Jaeger
        /// instance. For processes that make calls to non-instrumented services
        /// (third parties, SQL, legacy systems, etc.) the
        /// GenerateServiceSpecificBatches flag is provided to trick Jaeger into
        /// correctly detecting and displaying all dependent spans as if both
        /// sides reported data. For more details, see <a
        /// href="https://github.com/jaegertracing/jaeger-ui/issues/594">jaeger-ui/issues/594</a>.
        /// </remarks>
        public bool GenerateServiceSpecificBatches { get; set; } = true;

Examples

GenerateServiceSpecificBatches = true

GenerateServiceSpecificBatches = false

TODOs

• CHANGELOG.md updated for non-trivial changes
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #1498 (5c74376) into master (fa1e17f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
- Coverage   81.25%   81.25%   -0.01%     
==========================================
  Files         232      232              
  Lines        6312     6316       +4     
==========================================
+ Hits         5129     5132       +3     
- Misses       1183     1184       +1     

Impacted Files	Coverage Δ
...rc/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs	85.41% <100.00%> (+0.47%)	⬆️
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	100.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs	56.25% <0.00%> (-6.25%)	⬇️

[yurishkuro]

While I understand the desire behind this config option, I still think it's a bad thing for the SDK to support the ability to generate misleading data. I provided more reasoning in the ticket #1388 (comment)

[CodeBlanch]

@yurishkuro Did you see the images in the description? A thousand words, as they say. That test is mixing in instrumented and un-instrumented services. First one looks great, easy to understand, conveys a lot of meaning, shows relationships at a glance. The second one takes many more mental cycles to derive the same information.

[ndrwrbgs]

Yuri, does Jaeger only wish to support setups that have 100% tracing (all only in Jaeger)?

[reyang]

LGTM.

[cijothomas]

Given there is a concern that this causes exporter to send "incorrect" data, should we make this default to "false", so that SDK by default exports correct data. If a user want to send "incorrect" data to have better UI experience, let them explicitly opt-in to this behavior?

[yurishkuro]

I think the incorrect behavior should be removed altogether, without options to enable it. The Jaeger UI enhancement is already being added in jaegertracing/jaeger-ui#659

[CodeBlanch]

I'm going to verify everything works with the new Jaeger UI and then I'll open a PR to remove the hack. Question, is a note in CHANGELOG recommending users upgrade Jaeger UI to restore the functionality sufficient?