Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfluentKafka instrumentation #1493

Merged
merged 12 commits into from
Jul 2, 2024

Conversation

g7ed6e
Copy link
Contributor

@g7ed6e g7ed6e commented Dec 12, 2023

Fixes #1485.

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

Copy link

linux-foundation-easycla bot commented Dec 12, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does /p:IntegrationBuild=true do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It avoids taking a private dependency on MinVer. See

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

@eerhardt
Copy link
Contributor

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

Copy link

codecov bot commented Dec 15, 2023

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

Impacted file tree graph

@@            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
Copy link
Member

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
Copy link
Contributor Author

g7ed6e commented Dec 19, 2023

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.

@g7ed6e
Copy link
Contributor Author

g7ed6e commented Jun 26, 2024

@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
Copy link
Member

@g7ed6e
Copy link
Contributor Author

g7ed6e commented Jun 26, 2024

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

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CodeBlanch changed the title Add ConfluentKafka Add ConfluentKafka instrumentation Jun 27, 2024
@g7ed6e
Copy link
Contributor Author

g7ed6e commented Jun 28, 2024

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
Copy link
Member

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

@@ -43,6 +43,7 @@
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


```shell
dotnet add package OpenTelemetry.Instrumentation.ConfluentKafka --prerelease
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@g7ed6e
Copy link
Contributor Author

g7ed6e commented Jul 2, 2024

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

@Kielek I joined the open-telemetry organization.

@Kielek Kielek merged commit 92f4b83 into open-telemetry:main Jul 2, 2024
278 of 279 checks passed
@roseegalla
Copy link

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

@g7ed6e
Copy link
Contributor Author

g7ed6e commented Aug 7, 2024

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.confluentkafka Things related to OpenTelemetry.Instrumentation.ConfluentKafka documentation Improvements or additions to documentation infra Infra work - CI/CD, code coverage, linters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenTelemetry.Instrumentation.ConfluentKafka