Creating CompositePropagator #923

eddynaka · 2020-07-24T23:50:27Z

Fixes #582.

Changes

Changed the ITextFormat.Extract to accept ActivityContext based on the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#extract
Added CompositePropagator based on the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#create-a-composite-propagator
- Added tests
- removed duplicated (TraceContextActivityTest)

Thanks to @CodeBlanch and @cijothomas for the guidance!

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public API reviewed

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs

CodeBlanch

LGTM

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

src/OpenTelemetry.Api/Context/Propagation/ITextFormat.cs

src/OpenTelemetry.Api/Context/Propagation/TraceContextFormat.cs

cijothomas

We need to get more clarity either from this PR or from spec before merging.

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

src/OpenTelemetry.Api/Context/Propagation/ITextFormat.cs

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

cijothomas

My main concern is alleviated. I'd love to see more unit tests validating the behavior in all scenarios.
Also, this is a breaking change from beta - could you update changelog with this info?

codecov · 2020-07-29T05:16:56Z

Codecov Report

Merging #923 into master will increase coverage by 0.13%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master     #923      +/-   ##
==========================================
+ Coverage   68.28%   68.42%   +0.13%     
==========================================
  Files         219      220       +1     
  Lines        5976     5989      +13     
  Branches      978      981       +3     
==========================================
+ Hits         4081     4098      +17     
- Misses       1618     1619       +1     
+ Partials      277      272       -5

Impacted Files	Coverage Δ
...umentation.AspNet/Implementation/HttpInListener.cs	`0.00% <0.00%> (ø)`
...tation.AspNetCore/Implementation/HttpInListener.cs	`76.92% <50.00%> (-0.30%)`	⬇️
src/OpenTelemetry/Context/Propagation/B3Format.cs	`65.78% <66.66%> (ø)`
...etry.Api/Context/Propagation/TraceContextFormat.cs	`55.26% <72.22%> (+5.69%)`	⬆️
...try.Api/Context/Propagation/CompositePropagator.cs	`73.33% <73.33%> (ø)`
src/OpenTelemetry.Api/Trace/SpanContext.cs	`61.90% <100.00%> (+1.90%)`	⬆️
src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs	`96.96% <100.00%> (+12.59%)`	⬆️
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs	`80.00% <0.00%> (-13.34%)`	⬇️
... and 1 more

eddynaka · 2020-07-29T09:23:03Z

My main concern is alleviated. I'd love to see more unit tests validating the behavior in all scenarios.
Also, this is a breaking change from beta - could you update changelog with this info?

Sure! just updated and added a reference to spec

CodeBlanch · 2020-07-29T16:49:31Z

src/OpenTelemetry.Api/Trace/SpanContext.cs

+        /// Converts a <see cref="SpanContext"/> into an <see cref="ActivityContext"/>.
+        /// </summary>
+        /// <param name="spanContext"><see cref="SpanContext"/> source.</param>
+        public static implicit operator ActivityContext(SpanContext spanContext) => spanContext.ActivityContext;


@cijothomas I'm thinking about dropping implicit casts on all the shim classes. Why do that? Not sure. It just feels like a good thing to have. Could reduce bugs in places where we don't consider the Span case because we focus more on the Activity case? The idea is to allow Span version to be passed anywhere Activity version can be passed. How do you feel about that? 🤷

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CustomPropagator1.cs

src/OpenTelemetry.Api/CHANGELOG.md

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs

src/OpenTelemetry.Api/Context/Propagation/ITextFormat.cs

src/OpenTelemetry.Api/Context/Propagation/TraceContextFormat.cs

Adding ActivityContext to interface Adding tests Updating CompositePropagator + tests updating comments Improving performance when size is 0 updating tests updating default tracestate for empty MikeGoldsmith review adding more tests to exemplify usage Feature/composite propagator refactor (#1) * Added TestPropagator and switched a couple tests to use it. * removing extra classes Co-authored-by: Mikel Blanchard <[email protected]> updating changelog and constructor MikeGoldsmith comments updating logic

cijothomas · 2020-08-03T13:15:14Z

src/OpenTelemetry.Api/Context/Propagation/TraceContextFormat.cs

@@ -157,7 +156,7 @@ public void Inject<T>(ActivityContext activityContext, T carrier, Action<T, stri
            }
        }

-        private bool TryExtractTraceparent(string traceparent, out ActivityTraceId traceId, out ActivitySpanId spanId, out ActivityTraceFlags traceOptions)
+        internal static bool TryExtractTraceparent(string traceparent, out ActivityTraceId traceId, out ActivitySpanId spanId, out ActivityTraceFlags traceOptions)


Thanks to @CodeBlanch we'll get better help from .NET Activity API to do the parsing etc in the Rc1 release. We can refactor some of the code here at that time.

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

…ynaka/opentelemetry-dotnet into feature/composite-propagator

cijothomas · 2020-08-03T13:34:06Z

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

+            Assert.Contains(carrier, kv => kv.Key == "custom-traceparent");
+
+            bool isInjected = compositePropagator.IsInjected(carrier, Getter);
+            Assert.True(isInjected);


Lets validate that while injecting with 2 propagators using same key, the last one wins? There is no requirement that the 2nd one checks that header already exist and backs off right?

Same with extract - validate that we return when we find the 1st valid context.

For the inject part: ok, i did some changes that will validate the value and check if the latest one is the one that we have.
For the isInjected part: all will have to be injected. Since i did one with same header and one with different ones. i think we are covered.
For the extract part: changed the constructor to pass a boolean, so we can define if we will return a default or not. With that in mind, I counted how many times we used the Getter. So, if the first one returns default, we have to access only two times.

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs

This reverts commit 43ca7d0.

…ynaka/opentelemetry-dotnet into feature/composite-propagator

cijothomas · 2020-08-05T00:27:33Z

@MikeGoldsmith I merged this to make progress - please let us know if your comments are not addressed. We can follow up. Thanks!

eddynaka requested a review from a team July 24, 2020 23:50

CodeBlanch reviewed Jul 25, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs Outdated Show resolved Hide resolved

CodeBlanch approved these changes Jul 25, 2020

View reviewed changes

reyang added the pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package label Jul 25, 2020

CodeBlanch reviewed Jul 25, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 25, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/ITextFormat.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 25, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/TraceContextFormat.cs Show resolved Hide resolved

cijothomas requested changes Jul 25, 2020

View reviewed changes

CodeBlanch reviewed Jul 25, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs Show resolved Hide resolved

MikeGoldsmith suggested changes Jul 27, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs Outdated Show resolved Hide resolved

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 29, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/ITextFormat.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 29, 2020

View reviewed changes

src/OpenTelemetry.Api/Context/Propagation/CompositePropagator.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 29, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Outdated Show resolved Hide resolved

cijothomas requested changes Jul 29, 2020

View reviewed changes

cijothomas requested a review from MikeGoldsmith July 29, 2020 05:10

CodeBlanch reviewed Jul 29, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CustomPropagator1.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 29, 2020

View reviewed changes

src/OpenTelemetry.Api/CHANGELOG.md Show resolved Hide resolved

MikeGoldsmith suggested changes Jul 30, 2020

View reviewed changes

eddynaka and others added 5 commits July 31, 2020 07:31

Merge branch 'master' into feature/composite-propagator

b87baec

Merge branch 'master' into feature/composite-propagator

3746374

Merge branch 'master' into feature/composite-propagator

cea8121

Merge branch 'master' into feature/composite-propagator

d470999

cijothomas reviewed Aug 3, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Show resolved Hide resolved

cijothomas reviewed Aug 3, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Outdated Show resolved Hide resolved

checking value

022d28f

eddynaka added 2 commits August 3, 2020 10:33

adding inverted test

3a22fae

Merge branch 'feature/composite-propagator' of https://github.com/edd…

b71b3ca

…ynaka/opentelemetry-dotnet into feature/composite-propagator

cijothomas reviewed Aug 3, 2020

View reviewed changes

updating tests

59e8cb3

cijothomas reviewed Aug 3, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Show resolved Hide resolved

cijothomas reviewed Aug 3, 2020

View reviewed changes

test/OpenTelemetry.Tests/Implementation/Trace/Propagation/CompositePropagatorTest.cs Outdated Show resolved Hide resolved

eddynaka added 4 commits August 3, 2020 12:07

removing tracecontext and b3 tests

d7b00c5

testing only aspnet to merge

43ca7d0

Revert "testing only aspnet to merge"

b0956e1

This reverts commit 43ca7d0.

Merge branch 'master' into feature/composite-propagator

f003c0a

cijothomas approved these changes Aug 4, 2020

View reviewed changes

Merge branch 'master' into feature/composite-propagator

5d86a63

cijothomas requested a review from MikeGoldsmith August 4, 2020 13:44

eddynaka added 4 commits August 4, 2020 18:16

Merge branch 'master' into feature/composite-propagator

2664a1a

Merge branch 'feature/composite-propagator' of https://github.com/edd…

6265cf2

…ynaka/opentelemetry-dotnet into feature/composite-propagator

Merge branch 'master' into feature/composite-propagator

1856084

Merge branch 'master' into feature/composite-propagator

41f7e38

cijothomas merged commit 8948470 into open-telemetry:master Aug 5, 2020

reyang mentioned this pull request Aug 5, 2020

Need to rename BroadcastActivityProcessor #979

Closed

eddynaka deleted the feature/composite-propagator branch August 14, 2020 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating CompositePropagator #923

Creating CompositePropagator #923

eddynaka commented Jul 24, 2020

CodeBlanch left a comment

cijothomas left a comment

cijothomas left a comment

codecov bot commented Jul 29, 2020 •

edited

Loading

eddynaka commented Jul 29, 2020

CodeBlanch Jul 29, 2020

cijothomas Aug 3, 2020

cijothomas Aug 3, 2020

eddynaka Aug 3, 2020

cijothomas commented Aug 5, 2020

Creating CompositePropagator #923

Creating CompositePropagator #923

Conversation

eddynaka commented Jul 24, 2020

Changes

CodeBlanch left a comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2020 • edited Loading

Codecov Report

eddynaka commented Jul 29, 2020

CodeBlanch Jul 29, 2020

Choose a reason for hiding this comment

cijothomas Aug 3, 2020

Choose a reason for hiding this comment

cijothomas Aug 3, 2020

Choose a reason for hiding this comment

eddynaka Aug 3, 2020

Choose a reason for hiding this comment

cijothomas commented Aug 5, 2020

codecov bot commented Jul 29, 2020 •

edited

Loading