Add OTEL messaging semantic conventions to Kafka integration

[g7ed6e]

Description

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

Fixes # (issue)

Checklist

• Is this feature complete?
  • [x] Yes. Ready to ship.
  • [ ] No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • [ ] Yes
  • [x] No
• Did you add public API?
  • [x] 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
  • [x] No
• Does the change require an update in our Aspire docs?
  • [ ] Yes
    • Link to aspire-docs issue:
  • [ ] No

Microsoft Reviewers: Open in CodeFlow

[g7ed6e]

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

[mitchdenny]

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

Its importing now.

[g7ed6e]

/azp run

[azure-pipelines[bot]]

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

[g7ed6e]

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

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

[eerhardt]

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]

Can you update

https://github.com/dotnet/aspire/blob/62f8cf792deee900b8afd04c8e9e278c7dc6fa8a/src/Components/Telemetry.md?plain=1#L88-L105

[eerhardt]

Can you add some tests?

[davidfowl]

@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]

@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]

Can you update

https://github.com/dotnet/aspire/blob/62f8cf792deee900b8afd04c8e9e278c7dc6fa8a/src/Components/Telemetry.md?plain=1#L88-L105

done

[g7ed6e]

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.

[g7ed6e]

@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]

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

[eerhardt]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[eerhardt]

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 -

https://github.com/dotnet/aspire/blob/4e7e0de000bb83f824cca2773892a22b5da89245/src/Aspire.Hosting.Kafka/KafkaContainerImageTags.cs#L12-L15

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

https://github.com/dotnet/aspire/blob/4e7e0de000bb83f824cca2773892a22b5da89245/tests/Aspire.RabbitMQ.Client.Tests/RabbitMQContainerFixture.cs#L34-L38

[g7ed6e]

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 -

https://github.com/dotnet/aspire/blob/4e7e0de000bb83f824cca2773892a22b5da89245/src/Aspire.Hosting.Kafka/KafkaContainerImageTags.cs#L12-L15

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

https://github.com/dotnet/aspire/blob/4e7e0de000bb83f824cca2773892a22b5da89245/tests/Aspire.RabbitMQ.Client.Tests/RabbitMQContainerFixture.cs#L34-L38

@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:

1. drop the dependency on TestContainers.Kafka, pull TestContainer and implement our own KafakContainer and KafkaBuilder.
2. 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]

@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]

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

[g7ed6e]

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]

@eerhardt may i ask a review ?

[eerhardt]

@eerhardt may i ask a review ?

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

[davidfowl]

@lmolkova wheh you have some time can you look