Add ConfluentKafka instrumentation by g7ed6e · Pull Request #1493 · open-telemetry/opentelemetry-dotnet-contrib

g7ed6e · 2023-12-12T16:16:30Z

Changes

Add an instrumentation package for Confluent.Kafka library.
Implement tracing and propagation through Kafka message headers
Implement metrics

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

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

linux-foundation-easycla · 2023-12-12T16:16:34Z

The committers listed above are authorized under a signed CLA.

✅ login: g7ed6e / name: Guillaume Delahaye (9d0122c, bae86e9, b429a6f, 5afe9e0)
✅ login: CodeBlanch / name: Mikel Blanchard (f3c4df8, cdd2470, a4fc992, 9be7c18, 400fdc1, 4256bb1)
✅ login: vishweshbankwar / name: Vishwesh Bankwar (0f5bd95)
✅ login: Kielek / name: Piotr Kiełkowicz (14ad8c8)

eerhardt · 2023-12-15T16:44:06Z

+WORKDIR /repo
+COPY . ./
+WORKDIR "/repo/test/OpenTelemetry.Instrumentation.ConfluentKafka.Tests"
+RUN dotnet publish "OpenTelemetry.Instrumentation.ConfluentKafka.Tests.csproj" -c "${PUBLISH_CONFIGURATION}" -f "${PUBLISH_FRAMEWORK}" -o /drop -p:IntegrationBuild=true -p:TARGET_FRAMEWORK=${PUBLISH_FRAMEWORK}


What does /p:IntegrationBuild=true do?

It avoids taking a private dependency on MinVer. See

opentelemetry-dotnet-contrib/build/Common.prod.props

Line 23 in e42f6cd

<PackageReference Include="MinVer" Version="$(MinVerPkgVer)" Condition="'$(IntegrationBuild)' != 'true'">

eerhardt · 2023-12-15T17:34:21Z

On the Tracing side, would it be better if we just finished up confluentinc/confluent-kafka-dotnet#1838 (and also made a similar PR for Consumer)? That way we wouldn't need to wrap IProducer / IConsumer. The OpenTelemetry package would just enable the ActivitySource from the Kafka library.

cc @cijothomas

codecov · 2023-12-15T18:14:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.40%. Comparing base (71655ce) to head (14ad8c8).
Report is 349 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1493      +/-   ##
==========================================
- Coverage   73.91%   72.40%   -1.51%     
==========================================
  Files         267      304      +37     
  Lines        9615    11318    +1703     
==========================================
+ Hits         7107     8195    +1088     
- Misses       2508     3123     +615

Flag	Coverage Δ
unittests-Exporter.Geneva	`53.20% <ø> (?)`
unittests-Exporter.InfluxDB	`95.88% <ø> (?)`
unittests-Exporter.Instana	`71.24% <ø> (?)`
unittests-Exporter.OneCollector	`91.29% <ø> (?)`
unittests-Exporter.Stackdriver	`75.73% <ø> (?)`
unittests-Extensions	`79.33% <ø> (?)`
unittests-Extensions.AWS	`83.41% <ø> (?)`
unittests-Extensions.Enrichment	`100.00% <ø> (?)`
unittests-Instrumentation.AWS	`80.59% <ø> (?)`
unittests-Instrumentation.AWSLambda	`87.96% <ø> (?)`
unittests-Instrumentation.AspNet	`74.55% <ø> (?)`
unittests-Instrumentation.AspNetCore	`85.27% <ø> (?)`
unittests-Instrumentation.ElasticsearchClient	`79.87% <ø> (?)`
unittests-Instrumentation.EntityFrameworkCore	`55.49% <ø> (?)`
unittests-Instrumentation.EventCounters	`76.36% <ø> (?)`
unittests-Instrumentation.GrpcNetClient	`79.61% <ø> (?)`
unittests-Instrumentation.Hangfire	`93.58% <ø> (?)`
unittests-Instrumentation.Http	`81.08% <ø> (?)`
unittests-Instrumentation.Owin	`83.43% <ø> (?)`
unittests-Instrumentation.Process	`100.00% <ø> (?)`
unittests-Instrumentation.Quartz	`78.94% <ø> (?)`
unittests-Instrumentation.Runtime	`100.00% <ø> (?)`
unittests-Instrumentation.SqlClient	`90.90% <ø> (?)`
unittests-Instrumentation.StackExchangeRedis	`69.92% <ø> (?)`
unittests-Instrumentation.Wcf	`78.47% <ø> (?)`
unittests-PersistentStorage	`65.78% <ø> (?)`
unittests-Resources.AWS	`77.30% <ø> (?)`
unittests-Resources.Azure	`77.94% <ø> (?)`
unittests-Resources.Container	`72.41% <ø> (?)`
unittests-Resources.Gcp	`72.54% <ø> (?)`
unittests-Resources.Host	`52.87% <ø> (?)`
unittests-Resources.Process	`81.81% <ø> (?)`
unittests-Resources.ProcessRuntime	`82.35% <ø> (?)`
unittests-Sampler.AWS	`88.09% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 323 files with indirect coverage changes

cijothomas · 2023-12-15T18:16:22Z

On the Tracing side, would it be better if we just finished up confluentinc/confluent-kafka-dotnet#1838 (and also made a similar PR for Consumer)? That way we wouldn't need to wrap IProducer / IConsumer. The OpenTelemetry package would just enable the ActivitySource from the Kafka library.

cc @cijothomas

Yes. If that PR finishes, then there may not be even a need of an instrumentation library here, as users can just add AddSource themselves.

g7ed6e · 2023-12-19T18:57:09Z

On the Tracing side, would it be better if we just finished up confluentinc/confluent-kafka-dotnet#1838 (and also made a similar PR for Consumer)? That way we wouldn't need to wrap IProducer / IConsumer. The OpenTelemetry package would just enable the ActivitySource from the Kafka library.

cc @cijothomas

Yes. If that PR finishes, then there may not be even a need of an instrumentation library here, as users can just add AddSource themselves.

@eerhardt @cijothomas I agree but I think until that it would be great to get tracing support.

CodeBlanch · 2024-06-25T19:54:04Z

@g7ed6e I pushed some changes to try and get this over the finish line 😄

Missing MinVerTagPrefix in the .proj: f3c4df8
Static meters/counters: a4fc992

The way it was before users could end up with multiple meters in their process with the same name & version. That is a no-no in the OTel spec. SDK should actually be fine with it but it seemed simpler to just make them static.

Since I was in there I also switched the measurements to use TagList so they become allocation free. Before for every measurement it was allocating an enumerator and array. Expensive!
Lit up the integration tests in CI: 400fdc1

The tests are currently failing. Would you please look into that?

@Kielek @vishweshbankwar Some concerns with the tests being added on this PR. There are no unit tests, only integration tests. Usually we have a mix of both? Not a blocker for me so long as the CI runs what is there but I think ideally there would be some unit tests too. Could be a follow-up effort.

g7ed6e · 2024-06-26T07:58:21Z

@g7ed6e I pushed some changes to try and get this over the finish line 😄

Missing MinVerTagPrefix in the .proj: f3c4df8

Static meters/counters: a4fc992
The way it was before users could end up with multiple meters in their process with the same name & version. That is a no-no in the OTel spec. SDK should actually be fine with it but it seemed simpler to just make them static.
Since I was in there I also switched the measurements to use TagList so they become allocation free. Before for every measurement it was allocating an enumerator and array. Expensive!

Lit up the integration tests in CI: 400fdc1
The tests are currently failing. Would you please look into that?

@Kielek @vishweshbankwar Some concerns with the tests being added on this PR. There are no unit tests, only integration tests. Usually we have a mix of both? Not a blocker for me so long as the CI runs what is there but I think ideally there would be some unit tests too. Could be a follow-up effort.

@CodeBlanch I pulled your changes and pushed updates to the integration tests.

CodeBlanch · 2024-06-26T17:19:18Z

@g7ed6e Seems like the integration tests are still having an issue: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/9683812113/job/26720211596?pr=1493

g7ed6e · 2024-06-26T17:29:34Z

@g7ed6e Seems like the integration tests are still having an issue: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/9683812113/job/26720211596?pr=1493

~~@CodeBlanch it looks like multiple Kafka containers are created. I don't face this issue locally I will check CI scripts.~~

@CodeBlanch : I fixed and ran the integration tests locally using the compose file and that's ok. It should be fine in CI too.

CodeBlanch

I do still think we could do more to help connect traces when consuming (#1493 (comment)) but LGTM that could be done as follow-up work.

g7ed6e · 2024-06-28T12:50:21Z

I do still think we could do more to help connect traces when consuming (#1493 (comment)) but LGTM that could be done as follow-up work.

@CodeBlanch I missed that comment. I propose to implement it in a follow up PR.

CodeBlanch · 2024-06-28T17:03:10Z

@Kielek @vishweshbankwar I think this is good to go but since it is touching a lot of the repo I would like another maintainer to approve before merging.

vishweshbankwar · 2024-06-28T17:57:38Z

    <OpenTelemetryCoreLatestVersion>[1.9.0,2.0)</OpenTelemetryCoreLatestVersion>
    <OpenTelemetryCoreLatestPrereleaseVersion>[1.9.0-rc.1]</OpenTelemetryCoreLatestPrereleaseVersion>
    <StackExchangeRedisPkgVer>[2.1.58,3.0)</StackExchangeRedisPkgVer>
+    <ConfluentKafkaPkgVer>[2.3.0,3.0)</ConfluentKafkaPkgVer>


not a blocker - this could be bump to 2.4.0 now?

@vishweshbankwar Indeed 2.4.0 has been released since I started working on this. I will push the package update in one of the planned follow up PR.

vishweshbankwar · 2024-06-28T18:21:10Z

+
+```shell
+dotnet add package OpenTelemetry.Instrumentation.ConfluentKafka --prerelease
+```


It would be good to link the example from here. or have some basic examples on how to configure the instrumentation. (Not a blocker - can be a follow up PR)

vishweshbankwar

Looks good to me overall. Left couple minor comments that can be addressed in the follow up.

I do have the same concern as @CodeBlanch noted in #1493 (comment) about the lack of unit tests. Given this PR is already big, it can be addressed in a follow up.

Kielek

Good to merge when you join to open-telemetry organization.

g7ed6e · 2024-07-02T07:13:46Z

Good to merge when you join to open-telemetry organization.

@Kielek I joined the open-telemetry organization.

roseegalla · 2024-08-07T13:18:12Z

@Kielek and @g7ed6e are there any plans to publish this package?

g7ed6e · 2024-08-07T14:25:34Z

Sure. However this is not ready yet, there's still some work before publishing.

rgallagher-adc · 2024-09-23T13:23:17Z

+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
+    <TargetFrameworks>net8.0;net6.0;net462</TargetFrameworks>


@g7ed6e curious if there was a reason you didn't include .Net Standard 2.0? If not can it be added as a target framework?

g7ed6e had a problem deploying to external December 12, 2023 16:16 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from eb51913 to b68d8ff Compare December 12, 2023 16:18

g7ed6e had a problem deploying to external December 12, 2023 16:18 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from b68d8ff to 2c3f48d Compare December 12, 2023 16:21

g7ed6e had a problem deploying to external December 12, 2023 16:21 — with GitHub Actions Failure

eerhardt mentioned this pull request Dec 13, 2023

Add Kafka Component microsoft/aspire#951

Merged

g7ed6e mentioned this pull request Dec 14, 2023

Allow multiple calls to SetStatisticsHandler / SetLogHandler / SetErrorHandler confluentinc/confluent-kafka-dotnet#2155

Open

g7ed6e had a problem deploying to external December 14, 2023 15:10 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from 9aaf41e to ed91281 Compare December 14, 2023 15:48

g7ed6e had a problem deploying to external December 14, 2023 15:48 — with GitHub Actions Failure

g7ed6e had a problem deploying to external December 15, 2023 10:32 — with GitHub Actions Failure

eerhardt reviewed Dec 15, 2023

View reviewed changes

g7ed6e force-pushed the feature/confluent-kafka branch from 3c34dfe to dd97408 Compare December 16, 2023 14:53

g7ed6e had a problem deploying to external December 16, 2023 14:53 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from dd97408 to a81b49b Compare December 16, 2023 15:16

g7ed6e had a problem deploying to external December 16, 2023 15:16 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from a81b49b to 189f8a2 Compare December 16, 2023 16:42

g7ed6e had a problem deploying to external December 16, 2023 16:42 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from 189f8a2 to e3e63b6 Compare December 19, 2023 14:16

g7ed6e had a problem deploying to external December 19, 2023 14:16 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from e3e63b6 to a631e5c Compare December 19, 2023 16:12

g7ed6e had a problem deploying to external December 19, 2023 16:12 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from a631e5c to 8217902 Compare December 21, 2023 08:01

g7ed6e had a problem deploying to external December 21, 2023 08:01 — with GitHub Actions Failure

g7ed6e force-pushed the feature/confluent-kafka branch from 8217902 to 8c9513f Compare December 21, 2023 09:15

g7ed6e and others added 5 commits June 25, 2024 15:06

refactor

9d0122c

Add MinVerTagPrefix in kafka proj.

f3c4df8

Code review.

a4fc992

Add integration tests to CI.

400fdc1

Lint.

cdd2470

Fix integration tests

5afe9e0

CodeBlanch added 2 commits June 26, 2024 10:01

Merge branch 'main' into feature/confluent-kafka

9be7c18

Lint.

4256bb1

Fix docker-compose CI integration tests

b429a6f

CodeBlanch approved these changes Jun 27, 2024

View reviewed changes

g7ed6e mentioned this pull request Jun 28, 2024

[ConfluentKafka] Implement a process span to ease connecting traces #1932

Closed

vishweshbankwar reviewed Jun 28, 2024

View reviewed changes

Merge branch 'main' into feature/confluent-kafka

0f5bd95

vishweshbankwar reviewed Jun 28, 2024

View reviewed changes

vishweshbankwar approved these changes Jun 28, 2024

View reviewed changes

Merge branch 'main' into feature/confluent-kafka

14ad8c8

g7ed6e mentioned this pull request Jul 1, 2024

Add unit tests to ConfluentKafka instrumentration #1934

Open

Kielek approved these changes Jul 1, 2024

View reviewed changes

g7ed6e mentioned this pull request Jul 1, 2024

Add instructions to configure ConfluentKafka instrumentation #1935

Open

rgallagher-adc reviewed Sep 23, 2024

View reviewed changes

g7ed6e mentioned this pull request Nov 30, 2024

[feature request] Leave Kafka activities active after Consume is called #2351

Closed

Conversation

g7ed6e commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

linux-foundation-easycla bot commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt Dec 15, 2023

Choose a reason for hiding this comment

Uh oh!

g7ed6e Dec 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt commented Dec 15, 2023

Uh oh!

codecov bot commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cijothomas commented Dec 15, 2023

Uh oh!

g7ed6e commented Dec 19, 2023

Uh oh!

CodeBlanch commented Jun 25, 2024

Uh oh!

g7ed6e commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeBlanch commented Jun 26, 2024

Uh oh!

g7ed6e commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodeBlanch left a comment

Choose a reason for hiding this comment

Uh oh!

g7ed6e commented Jun 28, 2024

Uh oh!

CodeBlanch commented Jun 28, 2024

Uh oh!

vishweshbankwar Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

g7ed6e Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

vishweshbankwar Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

vishweshbankwar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kielek left a comment

Choose a reason for hiding this comment

Uh oh!

g7ed6e commented Jul 2, 2024

Uh oh!

roseegalla commented Aug 7, 2024

Uh oh!

g7ed6e commented Aug 7, 2024

Uh oh!

rgallagher-adc Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

g7ed6e commented Dec 12, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 15, 2023 •

edited

Loading

g7ed6e commented Jun 26, 2024 •

edited

Loading

g7ed6e commented Jun 26, 2024 •

edited

Loading

vishweshbankwar left a comment •

edited

Loading