feat: Add OpenTelemetry Instrumentation Provider #68

ashovlin · 2023-11-01T16:36:31Z

Issue #, if available: DOTNET-7031

Description of changes:

Adds AWS.Messaging.Telemetry.OpenTelemetry, which instruments the AWS Message Processing Framework for .NET for OpenTelemetry
- Based heavily on @normj 's prototype in https://github.com/awslabs/aws-dotnet-messaging/tree/normj/telemetry
Outside of OTel, the framework will now serialize MessageEnvelope.Metadata as top-level properties in the JSON envelope when sending messages. We're using this to telemetry, but us and customers can use for other CloudEvents extensions and/or custom attributes.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ashovlin · 2023-11-01T16:37:54Z

sampleapps/PublisherAPI/PublisherAPI.csproj

@@ -7,10 +7,13 @@
 </PropertyGroup>

 <ItemGroup>
+ <PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.6.0" />
+ <PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.6.0" />


The OpenTelemetry.Extensions.Hosting is necessary to use our new AddAWSMessagingInstrumentation extension method. Should it just be a dependency of our OTel package?

I don't think it's strictly required, see how we're setting up OTel during the unit tests via CreateTracerProviderBuilder() instead.

Is OpenTelemetry.Extensions.Hosting just something a user is going to have on their application that is doing the OTel configuration? Doesn't look like we have a compile dependency on the package.

OpenTelemetry.Extensions.Hosting enables IServiceCollection.AddOpenTelemetry, which is necessary to use our AddAWSMessagingInstrumentation extension that registers our OpenTelemetryProivder singleton and adds our source name.

But it is not strictly required, since you can also enable OpenTelemetry via CreateTracerProviderBuilder like we do in the tests. This is brought in via OpenTelemetry, which is a transitive dependency for the user via our new AWS.Messaging.Telemetry.OpenTelemetry.

// Alternative to DI, see https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/docs/trace/customizing-the-sdk/README.md#using-sdkcreatetracerproviderbuilder using (var tracerProvider = Sdk.CreateTracerProviderBuilder() .AddSource(TelemetryKeys.Source) .ConfigureResource(resource => resource.AddService("unittest")) .AddInMemoryExporter(activities).Build()) { await _publisher.PublishAsync(new ChatMessage { MessageDescription = "Test Description" }); }

So my thinking was that because the user isn't required to configure OTel via DI, we didn't want to require the dependency ourselves. Though on the other hand it is only ~44KB so if we think it'd be more convenient we could require it ourselves.

Lets start without it. Easier to add it then remove it.

ashovlin · 2023-11-01T16:42:57Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryProvider.cs

+ {
+ if (envelope.Metadata.TryGetValue(key, out var value))
+ { 
+ if (value is JsonElement jsonValue)


Norm's prototype introduced DictionaryStringObjectConverter, which was deserializing these back to strings.

I didn't implement that yet since we'd have to cover a lot of primitive types, so for now both customers and us will have to use JsonElement and know what type corresponds to which key. Let me know if you'd rather use Norm's approach though.

This also means the else if is dead code, had left it in from iterating on tests.

I would prefer we do some conversion. It seems confusing to users that in the publish they put in a string or number and then on the subscriber they type has changed. And it feels like we are leaking our serialization logic potentially into user's business code.

I attempted to apply the converter like the prototype, but realized via dotnet/runtime#32903 that a custom JsonConverter cannot deserialize the dictionary attributed with JsonExtensionData by design.

The prototype was working because it was doing something like this, so that MessageEnvelope.Metadata was not being treated as overflow via JsonExtensionData

{ "metadata": { "traceparent": "..." } }

But we want the OTel info (and possibly other extensions, set by us internally or users) in the top of the CloudEvents envelope.

We had an internal discussion on 11/9/23 debating:

Leave deserialization as-is (both us and users will have to work with JsonElement values) when handling Metadata. We’ll route MPF’s OTel info through this path.

Move the OTel properties to real properties in our MessageEnvelope<T>

Simplifies this PR since we're not working with overflow, but not user-provided extensions.

We also want to avoid any OTel code in the core AWS.Messaging package if possible.

We could use JsonExtensionData to initially de-serialize overflow into something like internal RawMetadata<string, JsonElement> and then do our own copy+casting into a separate public Metadata<string, object>.

We agreed to start with approach number 1 for now.

src/AWS.Messaging/MessageEnvelope.cs

test/AWS.Messaging.UnitTests/SerializationTests/EnvelopeSerializerTests.cs

src/AWS.Messaging/Telemetry/TelemetryKeys.cs

normj · 2023-11-01T20:44:10Z

sampleapps/PublisherAPI/PublisherAPI.csproj

@@ -7,10 +7,13 @@
 </PropertyGroup>

 <ItemGroup>
+ <PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.6.0" />
+ <PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.6.0" />


Is OpenTelemetry.Extensions.Hosting just something a user is going to have on their application that is doing the OTel configuration? Doesn't look like we have a compile dependency on the package.

normj · 2023-11-01T21:02:04Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryProvider.cs

+ {
+ if (envelope.Metadata.TryGetValue(key, out var value))
+ { 
+ if (value is JsonElement jsonValue)


I would prefer we do some conversion. It seems confusing to users that in the publish they put in a string or number and then on the subscriber they type has changed. And it feels like we are leaking our serialization logic potentially into user's business code.

src/AWS.Messaging.Telemetry.OpenTelemetry/README.md

src/AWS.Messaging/MessageEnvelope.cs

src/AWS.Messaging/Serialization/EnvelopeSerializer.cs

…ress other PR feedback

philasmar · 2023-11-15T14:32:57Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryProvider.cs

+ }
+
+ /// <summary>
+ /// Extracts propogation context from a <see cref="MessageEnvelope"/>, meant to be used with <see cref="TextMapPropagator"/>


typo in "propogation" should be "propagation"

Fixed in 7592368

philasmar · 2023-11-15T14:33:45Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryTrace.cs

+namespace AWS.Messaging.Telemetry.OpenTelemetry;
+
+/// <summary>
+/// An OpenTelemetry trace (wrapper around a <see cref="Activity"/>


you opened a paranthesis but didn't close it

Fixed in 7592368

philasmar · 2023-11-15T14:34:52Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryTrace.cs

+ contextToInject = _activity.Context;
+ }
+ // Even if an "AWS.Messaging" activity was not created, we still
+ // propogate the the current activity (if it exists) through the message envelope


you have 2 "the" in a row.

Fixed in 7592368

philasmar · 2023-11-15T14:35:46Z

src/AWS.Messaging.Telemetry.OpenTelemetry/OpenTelemetryTrace.cs

+ envelope.Metadata[key] = JsonSerializer.SerializeToElement(value);
+ }
+
+ private bool _disposed;


nit: add a new line between the property and the summary below

Fixed in 7592368

philasmar · 2023-11-15T14:40:09Z

test/AWS.Messaging.UnitTests/SerializationTests/EnvelopeSerializerTests.cs

@@ -362,7 +362,7 @@ public async Task SerializationCallbacks_AreCorrectlyInvoked()
 Assert.Equal("1.0", envelope.Version);
 Assert.Equal("/aws/messaging", envelope.Source?.ToString());
 Assert.Equal("addressInfo", envelope.MessageTypeIdentifier);
- Assert.Equal(true, envelope.Metadata["Is-Delivered"]);
+ Assert.Equal(true, envelope.Metadata["Is-Delivered"].GetBoolean());


could you please resolve the warnings?

I was taking a small moral stance here, since we weren't really asserting that "some condition is true", rather more like "this piece of data is equal to the value true".

But updated, not a big deal.

feat: Add OpenTelemetry Instrumentation Provider

e8cc349

ashovlin requested review from normj, 96malhar and philasmar November 1, 2023 16:36

ashovlin commented Nov 1, 2023

View reviewed changes

fix unit test after rebase for PR

6c42947

normj requested changes Nov 1, 2023

View reviewed changes

ashovlin added 2 commits November 10, 2023 11:55

Move MessageEnvelope.Metadata to use JsonElements explicitly, and add…

c455cc2

…ress other PR feedback

Move to file-scoped namespaces in new classes

a483042

ashovlin requested a review from normj November 10, 2023 17:12

normj approved these changes Nov 10, 2023

View reviewed changes

philasmar reviewed Nov 15, 2023

View reviewed changes

Documentation polish and fix test warning

7592368

philasmar approved these changes Nov 16, 2023

View reviewed changes

ashovlin merged commit 7a38d82 into dev Nov 16, 2023
5 checks passed

ashovlin deleted the shovlia/otel branch November 16, 2023 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add OpenTelemetry Instrumentation Provider #68

feat: Add OpenTelemetry Instrumentation Provider #68

ashovlin commented Nov 1, 2023

ashovlin Nov 1, 2023 •

edited

Loading

normj Nov 1, 2023

ashovlin Nov 9, 2023 •

edited

Loading

normj Nov 10, 2023

ashovlin Nov 1, 2023 •

edited

Loading

normj Nov 1, 2023

ashovlin Nov 10, 2023

normj Nov 1, 2023

normj Nov 1, 2023

philasmar Nov 15, 2023

ashovlin Nov 15, 2023

philasmar Nov 15, 2023

ashovlin Nov 15, 2023

philasmar Nov 15, 2023

ashovlin Nov 15, 2023

philasmar Nov 15, 2023

ashovlin Nov 15, 2023

philasmar Nov 15, 2023

ashovlin Nov 15, 2023

feat: Add OpenTelemetry Instrumentation Provider #68

feat: Add OpenTelemetry Instrumentation Provider #68

Conversation

ashovlin commented Nov 1, 2023

ashovlin Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin Nov 1, 2023 • 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

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

Choose a reason for hiding this comment

ashovlin Nov 1, 2023 •

edited

Loading

ashovlin Nov 9, 2023 •

edited

Loading

ashovlin Nov 1, 2023 •

edited

Loading