-
Notifications
You must be signed in to change notification settings - Fork 283
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
Conversation
eb51913
to
b68d8ff
Compare
b68d8ff
to
2c3f48d
Compare
9aaf41e
to
ed91281
Compare
src/OpenTelemetry.Instrumentation.ConfluentKafka/AssemblyInfo.cs
Outdated
Show resolved
Hide resolved
...Telemetry.Instrumentation.ConfluentKafka/OpenTelemetry.Instrumentation.ConfluentKafka.csproj
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.ConfluentKafka.Tests/Dockerfile
Outdated
Show resolved
Hide resolved
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} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'"> |
test/OpenTelemetry.Instrumentation.ConfluentKafka.Tests/docker-compose.yml
Outdated
Show resolved
Hide resolved
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 ReportAll modified and coverable lines are covered by tests ✅
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 Flags with carried forward coverage won't be shown. Click here to find out more. |
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. |
3c34dfe
to
dd97408
Compare
dd97408
to
a81b49b
Compare
a81b49b
to
189f8a2
Compare
189f8a2
to
e3e63b6
Compare
e3e63b6
to
a631e5c
Compare
@eerhardt @cijothomas I agree but I think until that it would be great to get tracing support. |
a631e5c
to
8217902
Compare
8217902
to
8c9513f
Compare
@CodeBlanch I pulled your changes and pushed updates to the integration tests. |
@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 : I fixed and ran the integration tests locally using the compose file and that's ok. It should be fine in CI too. |
There was a problem hiding this 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 I missed that comment. I propose to implement it in a follow up PR. |
@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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 | ||
``` |
There was a problem hiding this comment.
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)
There was a problem hiding this 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.
There was a problem hiding this 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.
@Kielek I joined the open-telemetry organization. |
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> |
There was a problem hiding this comment.
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?
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:
CHANGELOG.md
updated for non-trivial changes