Add OTEL messaging semantic conventions to Kafka integration #5765

g7ed6e · 2024-09-18T10:04:43Z

Description

Add OTEL messaging semantic conventions to Aspire.ConfluentKafka component/integration.

Fixes # (issue)

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue:
- No

Microsoft Reviewers: Open in CodeFlow

g7ed6e · 2024-09-18T10:07:13Z

@mitchdenny Could you add this package https://www.nuget.org/packages/OpenTelemetry.Instrumentation.ConfluentKafka/0.1.0-alpha.2 ?

mitchdenny · 2024-09-18T10:20:22Z

@mitchdenny Could you add this package https://www.nuget.org/packages/OpenTelemetry.Instrumentation.ConfluentKafka/0.1.0-alpha.2 ?

Its importing now.

g7ed6e · 2024-09-18T10:25:23Z

/azp run

azure-pipelines · 2024-09-18T10:25:29Z

Commenter does not have sufficient privileges for PR 5765 in repo dotnet/aspire

src/Aspire.Hosting/Aspire.Hosting.csproj

g7ed6e · 2024-09-18T12:04:40Z

@davidfowl / @eerhardt We now have 2 metrics sources.

Should we still emit both ? Should we add an AppContext.Switch ?

eerhardt · 2024-09-18T13:55:13Z

Should we still emit both ? Should we add an AppContext.Switch ?

I think we should start deprecating the existing one. Either removing it entirely now, or putting it behind an AppContext switch first, and then remove it later.

eerhardt · 2024-09-18T13:56:20Z

Can you update

aspire/src/Components/Telemetry.md

Lines 88 to 105 in 62f8cf7

    
           Aspire.Confluent.Kafka: 
        
           - Log categories: 
        
             - "Aspire.Confluent.Kafka" 
        
           - Activity source names: 
        
             - none (currently not supported by Confluent.Kafka library) 
        
           - Metric names: 
        
             - "Aspire.Confluent.Kafka" 
        
               - "messaging.kafka.consumer.queue.message_count" 
        
               - "messaging.kafka.producer.queue.message_count" 
        
               - "messaging.kafka.producer.queue.size" 
        
               - "messaging.kafka.network.tx"     
        
               - "messaging.kafka.network.transmitted" 
        
               - "messaging.kafka.network.rx" 
        
               - "messaging.kafka.network.received" 
        
               - "messaging.kafka.message.tx" 
        
               - "messaging.kafka.message.transmitted" 
        
               - "messaging.kafka.message.rx" 
        
               - "messaging.kafka.message.received"

eerhardt · 2024-09-18T13:57:37Z

Directory.Packages.props

@@ -139,6 +139,7 @@
    <PackageVersion Include="OpenTelemetry.Instrumentation.Http" Version="1.9.0" />
    <PackageVersion Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.9.0" />
    <PackageVersion Include="OpenTelemetry.Instrumentation.Runtime" Version="1.9.0" />
+    <PackageVersion Include="OpenTelemetry.Instrumentation.ConfluentKafka" Version="0.1.0-alpha.2" />


What's the plan for shipping this library stable? We won't be able to ship Aspire.Confluent.Kafka stable again until this package goes stable.

https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md and https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-metrics.md needs to be stable first. Than we can think about making this package stable.

I'm assuming that isn't going to happen this calendar year, is that correct?

If that is the case, maybe we need to think about "vendoring" this code and compiling it into the Aspire.Confluent.Kafka library like we do with SqlServer and Redis here:

https://github.com/dotnet/aspire/tree/main/src/Vendoring

So we're going to need to copy the implementation @eerhardt ?

I do not found any ETA for this. My feeling is more likely - early next year.

So we're going to need to copy the implementation @eerhardt ?

If we want to continue shipping "stable" versions of the Kafka client integration library.

Aspire.Component.Kafka is a bit more than 10k dl while Aspire.Hosting.Kafka is 7-8k. Aspire.Hosting.AppHost is 400-500k.

eerhardt · 2024-09-18T13:59:43Z

Can you add some tests?

davidfowl · 2024-09-30T17:55:22Z

@g7ed6e we're marching towards the date for Aspire 9 shutdown, are you interested in having this support land in 9? (I ask because I think this is a high value change for 9)

g7ed6e · 2024-09-30T18:17:47Z

@davidfowl Yes that would be great. I will try to copy/paste the code from the otel repo this week so we can drop the dependency to the alpha otel package.

g7ed6e · 2024-10-04T06:41:21Z

Can you update

aspire/src/Components/Telemetry.md

Lines 88 to 105 in 62f8cf7

Aspire.Confluent.Kafka:

- Log categories:

- "Aspire.Confluent.Kafka"

- Activity source names:

- none (currently not supported by Confluent.Kafka library)

- Metric names:

- "Aspire.Confluent.Kafka"

- "messaging.kafka.consumer.queue.message_count"

- "messaging.kafka.producer.queue.message_count"

- "messaging.kafka.producer.queue.size"

- "messaging.kafka.network.tx"

- "messaging.kafka.network.transmitted"

- "messaging.kafka.network.rx"

- "messaging.kafka.network.received"

- "messaging.kafka.message.tx"

- "messaging.kafka.message.transmitted"

- "messaging.kafka.message.rx"

- "messaging.kafka.message.received"

done

g7ed6e · 2024-10-04T06:41:37Z

Should we still emit both ? Should we add an AppContext.Switch ?

I think we should start deprecating the existing one. Either removing it entirely now, or putting it behind an AppContext switch first, and then remove it later.

done.
Behavior is that legacy metrics are no longer emitted, an AppContext switch let users re-enable these metrics. The switch is named EnableAspire8ConfluentKafkaMetrics.

eerhardt · 2024-10-09T15:41:28Z

tests/Aspire.Confluent.Kafka.Tests/.cs

+    [ModuleInitializer]
+    public static void Initialize()
+    {
+        AppContext.SetSwitch("EnableAspire8ConfluentKafkaMetrics", true);


we can do this in the runtimeconfig instead. See

aspire/playground/Directory.Build.props

Lines 21 to 25 in 72fe8bd



<ItemGroup>

<RuntimeHostConfigurationOption Include="Azure.Experimental.EnableActivitySource"

Value="true" />

</ItemGroup>

for an example how to do this.

eerhardt · 2024-10-09T15:41:53Z

tests/Aspire.Confluent.Kafka.Tests/.cs

@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.


If this file isn't removed, it should have a decent name.

it contains a ModuleInitializer that set the AppContext switch. How would you name it ? Program.cs ?

eerhardt · 2024-10-09T15:42:10Z

tests/Aspire.Confluent.Kafka.Tests/.cs

@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.


We need to add real tests here. Verify traces are working.

@eerhardt These tests would require a Kafka container running (this is how I wrote the tests in the OTEL repo). Is this what you would like to see here ?

Please see here https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/test/OpenTelemetry.Instrumentation.ConfluentKafka.Tests/HostedTracingAndMeteringTests.cs

Hum I did not noticed yet we were pulling TestContainers. Let's write integration tests :)

@eerhardt / @mitchdenny TestContainers.Kafka default image is confluentinc/cp-kafka:6.1.9 can you please:

add confluentinc/cp-kafka:6.1.9 to netaspireci.azurecr.io

add TestContainers.Kafka 3.10.0 to the nuget feed

edit: using the default TestContainers.Kafka default image will ease further maintenance of the UT.

g7ed6e · 2024-10-11T09:45:50Z

@davidfowl I updated the PR with integration tests similar to those present in otel repo, i hope we are still able to make this land for .NET9.

CI is failing but should work once:

confluentinc/cp-kafka:6.1.9 is added to netaspireci.azurecr.io registry
TestContainers.Kafka 3.10.0 to nuget feed

edit: /cc @mitchdenny / @eerhardt

mitchdenny · 2024-10-14T08:35:22Z

Testcontainers package should be in there in a few moments. I'm working on getting my access sorted for the container image.

eerhardt · 2024-10-14T16:35:06Z

/azp run

azure-pipelines · 2024-10-14T16:35:19Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt · 2024-10-14T16:38:17Z

confluentinc/cp-kafka:6.1.9 is added to netaspireci.azurecr.io registry

We shouldn't be using this container for tests. Instead we should use the Kafka container we reference in our Hosting package -

aspire/src/Aspire.Hosting.Kafka/KafkaContainerImageTags.cs

Lines 12 to 15 in 4e7e0de

    
           public const string Image = "confluentinc/confluent-local"; 
        
           /// <summary>7.7.1</summary> 
        
           public const string Tag = "7.7.1";

You can override the TestContainer's image like the following:

aspire/tests/Aspire.RabbitMQ.Client.Tests/RabbitMQContainerFixture.cs

Lines 34 to 38 in 4e7e0de

    
           public static async Task<RabbitMqContainer> CreateContainerAsync() 
        
           { 
        
               var container = new RabbitMqBuilder() 
        
                   .WithImage($"{TestConstants.AspireTestContainerRegistry}/{RabbitMQContainerImageTags.Image}:{RabbitMQContainerImageTags.Tag}") 
        
                   .Build();

g7ed6e · 2024-10-16T09:24:23Z

confluentinc/cp-kafka:6.1.9 is added to netaspireci.azurecr.io registry

We shouldn't be using this container for tests. Instead we should use the Kafka container we reference in our Hosting package -

aspire/src/Aspire.Hosting.Kafka/KafkaContainerImageTags.cs

Lines 12 to 15 in 4e7e0de

public const string Image = "confluentinc/confluent-local";

/// <summary>7.7.1</summary>

public const string Tag = "7.7.1";

You can override the TestContainer's image like the following:

aspire/tests/Aspire.RabbitMQ.Client.Tests/RabbitMQContainerFixture.cs

Lines 34 to 38 in 4e7e0de

public static async Task<RabbitMqContainer> CreateContainerAsync()

{

var container = new RabbitMqBuilder()

.WithImage($"{TestConstants.AspireTestContainerRegistry}/{RabbitMQContainerImageTags.Image}:{RabbitMQContainerImageTags.Tag}")

.Build();

@eerhardt confluentinc/cp-kafka:6.1.9 and confluentinc/confluent-local do not need the same env variables and startup script won't be compatible between the 2.
So we have 2 posibilities:

drop the dependency on TestContainers.Kafka, pull TestContainer and implement our own KafakContainer and KafkaBuilder.
use the community provided TestContainers.Kafka nuget package and get benefit on its probable future upgrade to confluentinc/confluent-local.

One thought: As the aim of these tests is to validate that the kafka clients behave correctly we could rely on whatever Kafka implementation and should not rely on the actual kafka server version. I mean that the aspire kafka integration should be compatible with others implementation like RedPanda for which a TestContainer.RedPanda NuGet package is published too.

Update: @eerhardt what do you think about not relying to a specific kafka version ?

g7ed6e · 2024-11-18T15:08:50Z

@eerhardt I've updated the PR adding a specific KafkaContainerBuilder for the requirements of the confluentinc/confluent-local:7.7.1 image. The implementation is inspired by the TestContainers go library which uses a confluentinc/confluent-local 7.x image too. (In opposition to the .NET one which uses a confluentinc/cp-kafka 6.x image).

g7ed6e · 2024-12-04T17:48:58Z

Hi @mitchdenny it looks like TestContainers.Kafka 4.0.0 package is missing in the feed. Could you please add it ?

eerhardt

Sorry for the delay @g7ed6e. This is looking really good. I just have a few comments. Once those are addressed I think we should be able to merge this.

I have added Testcontainers.Kafka 4.0.0 to our nuget mirror so CI should be unblocked.

src/Vendoring/README.md

src/Components/Aspire.Confluent.Kafka/KafkaConsumerSettings.cs

tests/Aspire.Confluent.Kafka.Tests/MeterProviderExtensions.cs

tests/Aspire.Confluent.Kafka.Tests/KafkaContainerFixture.cs

eerhardt · 2024-12-14T00:05:36Z

tests/Aspire.Confluent.Kafka.Tests/KafkaContainerFixture.cs

+        {
+            return base.Init()
+                .WithImage(KafkaImage)
+                .WithPortBinding(KafkaPort, true)


Do we need to copy all this from test containers? Can't we just override the Image like we do elsewhere?

aspire/tests/Aspire.RabbitMQ.Client.Tests/RabbitMQContainerFixture.cs

Lines 36 to 37 in 3991282

var container = new RabbitMqBuilder()

.WithImage($"{ComponentTestConstants.AspireTestContainerRegistry}/{RabbitMQContainerImageTags.Image}:{RabbitMQContainerImageTags.Tag}")

No we can't, that's why i first used the kafka 6.x images.

Please notice that client side packages should not be bound to a specific version of kafka. Confluent.Kafka producer and consumer can be used with other queue system that support the kafka protocol like redpanda or Azure Event Hub. Thus unit tests should not be tied to a specific service provider.

Add a specific KafkaContainerBuilder matching confluentinc/confluent-local:7.7.1 requirements Update tests/Aspire.Confluent.Kafka.Tests/MeterProviderExtensions.cs Co-authored-by: Eric Erhardt <[email protected]> Update ConfigurationSchema.json Update src/Vendoring/README.md Co-authored-by: Eric Erhardt <[email protected]> Use constants

g7ed6e · 2024-12-14T08:29:16Z

Sorry for the delay @g7ed6e. This is looking really good. I just have a few comments. Once those are addressed I think we should be able to merge this.

I have added Testcontainers.Kafka 4.0.0 to our nuget mirror so CI should be unblocked.

@eerhardt Thanks. CI is all green now. I updated the PR according to your comments and rebased main branch.

g7ed6e · 2025-01-10T08:14:10Z

@eerhardt may i ask a review ?

eerhardt · 2025-01-10T23:56:15Z

@eerhardt may i ask a review ?

Yep. Sorry was lost after coming back from the holidays. Looking now.

eerhardt

This looks great! Thanks for the great work and contribution here.

I just had one nit to follow coding conventions (use a property when used outside of the class). But I think this can be merged.

Since the PR validation hasn't run in a while, we should re-run with the current code just to make sure everything is still green.

src/Components/Aspire.Confluent.Kafka/ConfluentKafkaCommon.cs

…nfluentkafka

davidfowl · 2025-01-11T03:34:23Z

@lmolkova wheh you have some time can you look

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 18, 2024

g7ed6e changed the title ~~Add OTEL messaging semantic conventions~~ Add OTEL messaging semantic conventions to Kafka integration Sep 18, 2024

davidfowl reviewed Sep 18, 2024

View reviewed changes

src/Aspire.Hosting/Aspire.Hosting.csproj Outdated Show resolved Hide resolved

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from 10cb4cb to d19b556 Compare September 18, 2024 11:54

g7ed6e marked this pull request as ready for review September 18, 2024 12:06

g7ed6e requested review from radical, eerhardt and sebastienros as code owners September 18, 2024 12:06

eerhardt reviewed Sep 18, 2024

View reviewed changes

davidfowl added area-integrations Issues pertaining to Aspire Integrations packages kafka Issues related to Kafka integrations labels Sep 30, 2024

g7ed6e force-pushed the feature/add-otel-confluentkafka branch 2 times, most recently from ace513b to f9df469 Compare October 4, 2024 06:40

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from f9df469 to ae1f616 Compare October 9, 2024 15:15

eerhardt reviewed Oct 9, 2024

View reviewed changes

g7ed6e requested a review from eerhardt October 11, 2024 09:39

g7ed6e requested a review from davidfowl October 11, 2024 09:46

g7ed6e mentioned this pull request Oct 13, 2024

Fix Kafka integration documentation dotnet/docs-aspire#1821

Merged

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from 082e123 to 5e8ea6c Compare October 13, 2024 16:01

g7ed6e force-pushed the feature/add-otel-confluentkafka branch 2 times, most recently from 0350894 to d025618 Compare November 18, 2024 09:48

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from f0239c7 to 5b2218f Compare November 18, 2024 16:31

build-analysis bot mentioned this pull request Nov 18, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from 5b2218f to b1748b2 Compare November 27, 2024 15:17

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from b1748b2 to b283275 Compare December 4, 2024 16:43

eerhardt reviewed Dec 14, 2024

View reviewed changes

g7ed6e force-pushed the feature/add-otel-confluentkafka branch from 3077e79 to b0d74c3 Compare December 14, 2024 07:05

g7ed6e requested a review from eerhardt January 10, 2025 08:14

eerhardt approved these changes Jan 11, 2025

View reviewed changes

src/Components/Aspire.Confluent.Kafka/ConfluentKafkaCommon.cs Outdated Show resolved Hide resolved

eerhardt added 2 commits January 10, 2025 18:11

PR feedback. Use a property.

45b9dfe

Merge remote-tracking branch 'upstream/main' into feature/add-otel-co…

7ee8f34

…nfluentkafka

eerhardt enabled auto-merge (squash) January 11, 2025 00:14

eerhardt merged commit 7ed8d42 into dotnet:main Jan 11, 2025
9 checks passed

github-actions bot locked and limited conversation to collaborators Feb 10, 2025

	<!-- Enable Azure ActivitySources so Azure Components participate in tracing. -->
	<ItemGroup>
	<RuntimeHostConfigurationOption Include="Azure.Experimental.EnableActivitySource"
	Value="true" />
	</ItemGroup>

		@@ -0,0 +1,16 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -0,0 +1,16 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// The .NET Foundation licenses this file to you under the MIT license.

	var container = new RabbitMqBuilder()
	.WithImage($"{ComponentTestConstants.AspireTestContainerRegistry}/{RabbitMQContainerImageTags.Image}:{RabbitMQContainerImageTags.Tag}")

Add OTEL messaging semantic conventions to Kafka integration #5765

Add OTEL messaging semantic conventions to Kafka integration #5765

Conversation

g7ed6e commented Sep 18, 2024 • edited Loading

Description

Checklist

Microsoft Reviewers: Open in CodeFlow

g7ed6e commented Sep 18, 2024

mitchdenny commented Sep 18, 2024

g7ed6e commented Sep 18, 2024

azure-pipelines bot commented Sep 18, 2024

g7ed6e commented Sep 18, 2024

eerhardt commented Sep 18, 2024

eerhardt commented Sep 18, 2024

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

g7ed6e Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

eerhardt commented Sep 18, 2024

davidfowl commented Sep 30, 2024 • edited Loading

g7ed6e commented Sep 30, 2024

g7ed6e commented Oct 4, 2024

g7ed6e commented Oct 4, 2024 • 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

g7ed6e Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

g7ed6e commented Oct 11, 2024 • edited Loading

mitchdenny commented Oct 14, 2024

eerhardt commented Oct 14, 2024

azure-pipelines bot commented Oct 14, 2024

eerhardt commented Oct 14, 2024

g7ed6e commented Oct 16, 2024 • edited Loading

g7ed6e commented Nov 18, 2024 • edited Loading

g7ed6e commented Dec 4, 2024 • edited Loading

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g7ed6e Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

g7ed6e commented Dec 14, 2024

g7ed6e commented Jan 10, 2025

eerhardt commented Jan 10, 2025

eerhardt left a comment

Choose a reason for hiding this comment

davidfowl commented Jan 11, 2025

g7ed6e commented Sep 18, 2024 •

edited

Loading

g7ed6e Sep 19, 2024 •

edited

Loading

davidfowl commented Sep 30, 2024 •

edited

Loading

g7ed6e commented Oct 4, 2024 •

edited

Loading

g7ed6e Oct 10, 2024 •

edited

Loading

g7ed6e commented Oct 11, 2024 •

edited

Loading

g7ed6e commented Oct 16, 2024 •

edited

Loading

g7ed6e commented Nov 18, 2024 •

edited

Loading

g7ed6e commented Dec 4, 2024 •

edited

Loading

g7ed6e Dec 14, 2024 •

edited

Loading