KAFKA-15668: Adding Opentelmetry shadowed library (KIP-714) #14618

apoorvmittal10 · 2023-10-23T16:55:50Z

The PR comprises of changes to include opentlemetry library as defined in KIP-714. The libraries are shadowed to prevent conflicts.

jar -tf kafka-clients-3.7.0-SNAPSHOT.jar | grep google
org/apache/kafka/shaded/com/google/
org/apache/kafka/shaded/com/google/protobuf/
org/apache/kafka/shaded/com/google/protobuf/AbstractMessage$Builder.class
org/apache/kafka/shaded/com/google/protobuf/AbstractMessage$BuilderParent.class
org/apache/kafka/shaded/com/google/protobuf/AbstractMessage.class
org/apache/kafka/shaded/com/google/protobuf/AbstractMessageLite$Builder$LimitedInputStream.class
org/apache/kafka/shaded/com/google/protobuf/AbstractMessageLite$Builder.class
org/apache/kafka/shaded/com/google/protobuf/AbstractMessageLite$InternalOneOfEnum.class
.
.

jar -tf kafka-clients-3.7.0-SNAPSHOT.jar | grep opentelemetry
org/apache/kafka/shaded/io/opentelemetry/
org/apache/kafka/shaded/io/opentelemetry/proto/
org/apache/kafka/shaded/io/opentelemetry/proto/resource/
org/apache/kafka/shaded/io/opentelemetry/proto/resource/v1/
org/apache/kafka/shaded/io/opentelemetry/proto/resource/v1/ResourceProto.class
org/apache/kafka/shaded/io/opentelemetry/proto/resource/v1/Resource$1.class
org/apache/kafka/shaded/io/opentelemetry/proto/resource/v1/Resource$Builder.class
org/apache/kafka/shaded/io/opentelemetry/proto/resource/v1/ResourceOrBuilder.class
.
.
.

https://central.sonatype.com/artifact/org.apache.kafka/kafka-clients/3.6.0/overview details the 3.6.0 java client pom with runtime dependencies and current changes maintains that behaviour. Outputting the pom below:

cat ~/.m2/repository/org/apache/kafka/kafka-clients/3.7.0-SNAPSHOT/kafka-clients-3.7.0-SNAPSHOT.pom
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <modelVersion>4.0.0</modelVersion>
  <groupId>org.apache.kafka</groupId>
  <artifactId>kafka-clients</artifactId>
  <version>3.7.0-SNAPSHOT</version>
  <name>Apache Kafka</name>
  <url>https://kafka.apache.org</url>
  <licenses>
    <license>
      <name>The Apache License, Version 2.0</name>
      <url>http://www.apache.org/licenses/LICENSE-2.0.txt</url>
      <distribution>repo</distribution>
    </license>
  </licenses>
  <dependencies>
    <dependency>
      <groupId>com.github.luben</groupId>
      <artifactId>zstd-jni</artifactId>
      <version>1.5.5-6</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>org.lz4</groupId>
      <artifactId>lz4-java</artifactId>
      <version>1.8.0</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>org.xerial.snappy</groupId>
      <artifactId>snappy-java</artifactId>
      <version>1.1.10.5</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
      <version>1.7.36</version>
      <scope>runtime</scope>
    </dependency>
    <dependency>
      <groupId>io.opentelemetry.proto</groupId>
      <artifactId>opentelemetry-proto</artifactId>
      <version>1.0.0-alpha</version>
      <scope>runtime</scope>
    </dependency>
  </dependencies>
</project>

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

AndrewJSchofield

lgtm

apoorvmittal10 · 2023-10-24T09:29:55Z

Build is failing because of flaky tests:

5 tests have failed
There are 0 new tests failing, 5 existing failing and 152 skipped.


Existing failures - 5
Build / JDK 8 and Scala 2.12 / testSslClientAuthRequestedOverriddenForSaslSslListener() – org.apache.kafka.common.security.authenticator.SaslAuthenticatorTest
1s
Build / JDK 8 and Scala 2.12 / testCorrectnessForCacheAndIndexFilesWhenResizeCache() – kafka.log.remote.RemoteIndexCacheTest
<1s
Build / JDK 17 and Scala 2.13 / testMultiWorkerRestartOnlyConnector – org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest
2m 23s
Build / JDK 17 and Scala 2.13 / testDescribeClusterRequestExcludingClusterAuthorizedOperations(String).quorum=kraft – kafka.server.DescribeClusterRequestTest
4s
Build / JDK 17 and Scala 2.13 / [2] Type=Raft-Isolated, Name=testDescribeQuorumStatusSuccessful, MetadataVersion=3.7-IV1, Security=PLAINTEXT – org.apache.kafka.tools.MetadataQuorumCommandTest
1m 8s

mjsax

I am not a gradle expert. Maybe @ijuma can take a look?

mjsax · 2023-10-27T02:49:55Z

build.gradle

+          if (!shouldPublishWithShadow) {
+            from components.java
+          } else {
+            apply plugin: 'com.github.johnrengelman.shadow'


What is this plugin? Does not look like something official? Not sure if we should use something that is no backed by some official OSS project?

Valid ask, we require this plugin to move Opentelemetry required libs as shadowed to avoid version conflicts when higher version of these commonly used libraries are present in user's application code, but this link for the plugin details better. This dependency is not introduced in this PR rather being already used in Kafka for a while:

kafka/build.gradle

Line 47 in f0e9739

id 'com.github.johnrengelman.shadow' version '8.1.1' apply false

Thanks. Yes, I understood already what the plugin does.

This dependency is not introduced in this PR rather being already used in Kafka for a while:

This was my concern, but if it's already established my concerns are void.

build.gradle

xvrl · 2023-10-27T22:04:43Z

@apoorvmittal10 could we show a diff before/after this change of:

the pom
the content of the jar – excluding org/apache/kafka/shaded/com/google/protobuf/ and org/apache/kafka/shaded/io/opentelemetry/proto/

to make sure the only difference in the resulting artifact are those shaded classes?

build.gradle

apoorvmittal10 · 2023-10-28T00:48:43Z

@apoorvmittal10 could we show a diff before/after this change of:

the pom

the content of the jar – excluding org/apache/kafka/shaded/com/google/protobuf/ and org/apache/kafka/shaded/io/opentelemetry/proto/

to make sure the only difference in the resulting artifact are those shaded classes?

@xvrl Please find the details below, AK trunk branch build vs changes in PR. Do you think we should relocate *.proto files too, didn't do that as I felt moving classes should be sufficient?

➜  3.7.0-SNAPSHOT-trunk> jar -tf kafka-clients-3.7.0-SNAPSHOT.jar  > jar_tf_output
➜  3.7.0-SNAPSHOT-changes>  jar -tf kafka-clients-3.7.0-SNAPSHOT.jar | grep -v org/apache/kafka/shaded/  > jar_tf_output

git diff 3.7.0-SNAPSHOT-trunk/jar_tf_output 3.7.0-SNAPSHOT-changes/jar_tf_output

git diff 3.7.0-SNAPSHOT-trunk/kafka-clients-3.7.0-SNAPSHOT.pom 3.7.0-SNAPSHOT-changes/kafka-clients-3.7.0-SNAPSHOT.pom

xvrl · 2023-10-31T21:44:34Z

@xvrl Please find the details below, AK trunk branch build vs changes in PR. Do you think we should relocate *.proto files too, didn't do that as I felt moving classes should be sufficient?

I think we should minimize the differences. Unless the protos are actually necessary at runtime, I think we should omit them to avoid polluting the jar with unnecessary resources.

git diff 3.7.0-SNAPSHOT-trunk/kafka-clients-3.7.0-SNAPSHOT.pom 3.7.0-SNAPSHOT-changes/kafka-clients-3.7.0-SNAPSHOT.pom

It looks like we are adding opentelemetry-proto as a runtime dependency, that doesn't look right. Since the proto dependencies are already included in the jar and relocated, the resulting pom should be identical.

xvrl

looks like the pom file is still defining unnecessary runtime dependencies, and ideally we do not add extraneous resource files to the jar.

apoorvmittal10 · 2023-10-31T23:43:00Z

jar -tf kafka-clients-3.7.0-SNAPSHOT.jar | grep -v org/apache/kafka/shaded/ > jar_tf_output

I have updated the build file and below are the results; tldr there is no change in pom and jar other than shaded libs.

git diff 3.7.0-SNAPSHOT-trunk/kafka-clients-3.7.0-SNAPSHOT.pom 3.7.0-SNAPSHOT-changes-final/kafka-clients-3.7.0-SNAPSHOT.pom

jar changes from last commit

build.gradle

xvrl

one minor suggestion, otherwise LGTM

Co-authored-by: Xavier Léauté <xl+github@xvrl.net>

apoorvmittal10 · 2023-11-01T15:55:52Z

one minor suggestion, otherwise LGTM

Thanks @xvrl, I have added the suggestion.

apoorvmittal10 · 2023-11-01T23:41:16Z

@wcarlson5 @mjsax 16 tests unrelated to the PR are failing. As we have +1 from @xvrl hence can we please merge this PR now. Thanks for the help.

mjsax · 2023-11-02T04:54:00Z

Thanks for all the review help @xvrl!

Merged to trunk.

…4618) Part of KIP-714. The PR comprises of changes to include opentlemetry library as defined in KIP-714. The libraries are shadowed to prevent conflicts. Co-authored-by: Xavier Léauté <xl+github@xvrl.net> Reviewers: Andrew Schofield <aschofield@confluent.io>, Xavier Léauté <xavier@confluent.io>, Walker Carlson <wcarlson@confluent.io>, Matthias J. Sax <matthias@confluent.io>

KAFKA-15668: Adding Opentelmetry shadowed library (KIP-714)

71db21c

apoorvmittal10 mentioned this pull request Oct 23, 2023

KAFKA-15618: Kafka metrics collector and supporting classes (KIP-714) #14620

Merged

3 tasks

AndrewJSchofield approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'trunk' into kip-714-ak-lib

503dd84

mjsax added kip Requires or implements a KIP dependencies Pull requests that update a dependency file labels Oct 27, 2023

mjsax reviewed Oct 27, 2023

View reviewed changes

apoorvmittal10 requested a review from mjsax October 27, 2023 10:36

xvrl reviewed Oct 27, 2023

View reviewed changes

build.gradle Show resolved Hide resolved

xvrl reviewed Oct 27, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

xvrl reviewed Oct 27, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

xvrl reviewed Oct 27, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

apoorvmittal10 added 2 commits October 28, 2023 01:34

Addressing review comments

4737f16

Merge remote-tracking branch 'upstream/trunk' into kip-714-ak-lib

70bde16

apoorvmittal10 requested a review from xvrl October 28, 2023 00:50

wcarlson5 approved these changes Oct 31, 2023

View reviewed changes

xvrl suggested changes Oct 31, 2023

View reviewed changes

Removing proto files and runtime dependency

ff85180

apoorvmittal10 requested a review from xvrl October 31, 2023 23:43

xvrl reviewed Nov 1, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

xvrl approved these changes Nov 1, 2023

View reviewed changes

Accepting suggestion

302707f

Co-authored-by: Xavier Léauté <xl+github@xvrl.net>

mjsax merged commit b850b46 into apache:trunk Nov 2, 2023

KAFKA-15668: Adding Opentelmetry shadowed library (KIP-714) #14618

KAFKA-15668: Adding Opentelmetry shadowed library (KIP-714) #14618

Uh oh!

Conversation

apoorvmittal10 commented Oct 23, 2023

Committer Checklist (excluded from commit message)

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 commented Oct 24, 2023

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

mjsax Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xvrl commented Oct 27, 2023

Uh oh!

Uh oh!

Uh oh!

apoorvmittal10 commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xvrl commented Oct 31, 2023

Uh oh!

xvrl left a comment

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 commented Oct 31, 2023

Uh oh!

Uh oh!

xvrl left a comment

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 commented Nov 1, 2023

Uh oh!

apoorvmittal10 commented Nov 1, 2023

Uh oh!

mjsax commented Nov 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

apoorvmittal10 commented Oct 28, 2023 •

edited

Loading