Allow multiple context propagators #104

RassK · 2021-04-05T12:06:48Z

Background

This PR adds support for multiple propagators that's required by the OTel standard. (following comment in PR #92)

What's next

Configuration source needs to be validated in early startup time. After that point settings instance should be considered as guaranteed OK (no throwing later in run time). See comments:

RassK · 2021-04-05T12:13:35Z

src/Datadog.Trace/Tracer.cs

@@ -119,7 +119,8 @@ internal Tracer(TracerSettings settings, ITraceWriter traceWriter, ISampler samp
            _scopeManager = scopeManager ?? new AsyncLocalScopeManager();
            Sampler = sampler ?? new RuleBasedSampler(new RateLimiter(Settings.MaxTracesSubmittedPerSecond));

-            _propagator = ContextPropagatorBuilder.BuildPropagator(Settings.Propagator, TraceIdConvention);
+            var propagators = ContextPropagatorBuilder.BuildPropagators(Settings.Propagators, TraceIdConvention);


Please note the upcoming configuration issue here. PropagatorType should be matched with TraceIdConvention (eg: we can go with name:value based configuration key), cause single convention type could not be accepted for all of the propagators. (See PR #102 comment)

Looking for your thoughts!

#102 (comment)

Current result: no support for different TraceIds in same time

src/Datadog.Trace/Propagation/MultiplexSpanContextPropagator.cs

src/Datadog.Trace/Configuration/ConfigurationSourceExtensions.cs

src/Datadog.Trace/Propagation/MultiplexSpanContextPropagator.cs

src/Datadog.Trace/Propagation/ContextPropagatorBuilder.cs

src/Datadog.Trace/Propagation/MultiplexSpanContextPropagator.cs

pellared · 2021-04-06T17:43:17Z

src/Datadog.Trace/Configuration/ConfigurationSourceExtensions.cs

+            return typedValue;
+        }
+
+        public static IEnumerable<TEnum> GetTypedValues<TEnum>(this IConfigurationSource source, string key)


how we would know that an invalid value was provided. more context

I'm not sure this is the right place to deal with validation. As mentioned in #102 it should fail fast. I would propose configuration source validator in early startup (binding configuration to settings instance). After that point configuration should be valid for whole application.

For now, would you consider making this use Enum.Parse and throw an exception so we know that the result is valid? I'd prefer that this PR close with a clear indication of configuration errors, so this change might be easier than doing a configuration source validator right now

Originally I tried not to change parsing logic with this PR (notice the previous logic is 1:1 same as now) but I can easily change it there is interest.

I suggest doing it separately.

pellared · 2021-04-06T17:43:34Z

src/Datadog.Trace/Configuration/ConfigurationSourceExtensions.cs

+                ?? Enumerable.Empty<string>();
+        }
+
+        public static TEnum GetTypedValue<TEnum>(this IConfigurationSource source, string key)


how we would know that an invalid value was provided. more context

src/Datadog.Trace/Propagation/CompositeTextMapPropagator.cs

src/Datadog.Trace/Propagation/ContextPropagatorFactory.cs

pellared · 2021-04-06T17:55:10Z

test/Datadog.Trace.Tests/Propagators/B3SpanContextPropagatorTests.cs

-
-        private static void AssertExpected(IHeadersCollection headers, string key, string expected)
-        {
-            var matches = headers.GetValues(key);
-            Assert.Single(matches);
-            matches.ToList().ForEach(x => Assert.Equal(expected, x));
-        }
-
-        private static void AssertMissing(IHeadersCollection headers, string key)
-        {
-            var matches = headers.GetValues(key);
-            Assert.Empty(matches);
-        }


why these methods have been moved?

Reused for CompositeTextMapPropagatorTests, HeadersCollectionTestBase seems to be ideal for tests that deal with headers.

I see. Only AssertMissing is not reused there but I think we can keep it as you refactored 👍

Yep, both seemed to be generic / reusable. Seems bit weird to leave one out 😄

src/Datadog.Trace/Configuration/TracerSettings.cs

pellared

Approving. It will be good to make some follow-up on https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/104/files#r608056476

zacharycmontoya · 2021-04-09T18:26:39Z

test/Datadog.Trace.TestHelpers/HeadersCollectionTestBase.cs

@@ -29,5 +30,18 @@ public static IEnumerable<object[]> GetHeadersInvalidSamplingPrioritiesCartesian
                   from invalidSamplingPriority in HeadersCollectionTestHelpers.GetInvalidSamplingPriorities().SelectMany(i => i)
                   select new[] { header, invalidSamplingPriority };
        }
+
+        internal static void AssertExpected(IHeadersCollection headers, string key, string expected)


You could make this public so you don't have to add an InternalsVisibleTo attribute. This won't be consumed by developers so I don't see any reason to keep this internal

IHeadersCollection is also internal, that's the original reason.

zacharycmontoya · 2021-04-09T18:46:25Z

src/Datadog.Trace/Propagation/CompositeTextMapPropagator.cs

+    /// </summary>
+    internal class CompositeTextMapPropagator : IPropagator
+    {
+        private readonly ICollection<IPropagator> _propagators;


Could this be an IEnumerable<IPropagator> instead? It looks like this is only used to enumerate values and doesn't do insertions/removals

I usually like to use ICollection when list is actually materialized - so it's quickly visible it's safe to use. With IEnumerable you are also introducing deferred execution, it can easily backfire with context like this.
(for eg if you happen to connect factory with deferred execution, it's perfectly valid code vise, but introduces a major performance loss - so ICollection is also more like a code guard)

In perfect world I would use IReadOnlyCollection, but here it doesn't seem to be a pattern.

I agree with @RassK . I am used to use IReadOnlyCollection for such cases . It is IMO more readable as well. Also Resharper and Rider static analysis often stops complaining about multiple enumeration in such cases

zacharycmontoya · 2021-04-13T18:35:49Z

Unit tests failed because of flaky test: Datadog.Trace.Tests.SpanTests.Accurate_Duration. Datadog just merged DataDog/dd-trace-dotnet#1360 that hopefully resolves this permanently

zacharycmontoya · 2021-04-13T18:39:12Z

Since the test failure is known to be flaky, I'll go ahead and merge

RassK commented Apr 5, 2021

View reviewed changes

RassK marked this pull request as ready for review April 5, 2021 13:21

RassK requested a review from a team April 5, 2021 13:21

nrcventura reviewed Apr 5, 2021

View reviewed changes

src/Datadog.Trace/Propagation/MultiplexSpanContextPropagator.cs Outdated Show resolved Hide resolved

pellared reviewed Apr 6, 2021

View reviewed changes

src/Datadog.Trace/Propagation/CompositeTextMapPropagator.cs Outdated Show resolved Hide resolved

pellared reviewed Apr 6, 2021

View reviewed changes

src/Datadog.Trace/Propagation/CompositeTextMapPropagator.cs Outdated Show resolved Hide resolved

pellared reviewed Apr 6, 2021

View reviewed changes

src/Datadog.Trace/Propagation/ContextPropagatorFactory.cs Outdated Show resolved Hide resolved

pellared reviewed Apr 6, 2021

View reviewed changes

zacharycmontoya reviewed Apr 6, 2021

View reviewed changes

src/Datadog.Trace/Configuration/TracerSettings.cs Outdated Show resolved Hide resolved

pellared approved these changes Apr 7, 2021

View reviewed changes

RassK mentioned this pull request Apr 7, 2021

Configuration source binding & validation #115

Closed

RassK closed this Apr 8, 2021

RassK reopened this Apr 8, 2021

RassK added 6 commits April 9, 2021 13:30

Allow multiple context propagators

7ac030d

CR refactor namings and helpers

f43d62f

Fix unit tests

cdbed5e

change accessibility to private

12f31b9

semantics fix

cdd575b

format code

701847b

RassK force-pushed the context-multi-propagation branch from eb98a7e to 701847b Compare April 9, 2021 10:42

RassK closed this Apr 9, 2021

RassK reopened this Apr 9, 2021

zacharycmontoya reviewed Apr 9, 2021

View reviewed changes

nrcventura approved these changes Apr 12, 2021

View reviewed changes

Merge branch 'main' into context-multi-propagation

aecc0ef

RassK closed this Apr 13, 2021

RassK reopened this Apr 13, 2021

pjanotti approved these changes Apr 13, 2021

View reviewed changes

zacharycmontoya approved these changes Apr 13, 2021

View reviewed changes

Merge branch 'main' into context-multi-propagation

448f8e9

zacharycmontoya merged commit 4de70b3 into open-telemetry:main Apr 13, 2021

RassK deleted the context-multi-propagation branch April 14, 2021 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple context propagators #104

Allow multiple context propagators #104

RassK commented Apr 5, 2021 •

edited

Loading

RassK Apr 5, 2021 •

edited

Loading

pellared Apr 6, 2021

RassK Apr 6, 2021

pellared Apr 6, 2021

RassK Apr 6, 2021

zacharycmontoya Apr 9, 2021

RassK Apr 10, 2021

pellared Apr 10, 2021 •

edited

Loading

pellared Apr 6, 2021

pellared Apr 6, 2021

RassK Apr 6, 2021

pellared Apr 6, 2021

RassK Apr 7, 2021

pellared left a comment

zacharycmontoya Apr 9, 2021

RassK Apr 10, 2021

zacharycmontoya Apr 9, 2021

RassK Apr 10, 2021 •

edited

Loading

RassK Apr 10, 2021

pellared Apr 10, 2021 •

edited

Loading

zacharycmontoya commented Apr 13, 2021

zacharycmontoya commented Apr 13, 2021

Allow multiple context propagators #104

Allow multiple context propagators #104

Conversation

RassK commented Apr 5, 2021 • edited Loading

Background

What's next

RassK Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RassK Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

zacharycmontoya commented Apr 13, 2021

zacharycmontoya commented Apr 13, 2021

RassK commented Apr 5, 2021 •

edited

Loading

RassK Apr 5, 2021 •

edited

Loading

pellared Apr 10, 2021 •

edited

Loading

RassK Apr 10, 2021 •

edited

Loading

pellared Apr 10, 2021 •

edited

Loading