-
Notifications
You must be signed in to change notification settings - Fork 615
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
Feature/repairkafka3.7 #707
Changes from 14 commits
3feef5a
2971c2b
7db4f51
1cbc3db
6220bd4
1e2b82a
d875bd3
ef2193b
e20ef76
3543204
526abbb
abd3848
0cb524c
f31fb6f
8297b83
73c85a0
1cc9438
960be6c
14ebf37
39353e8
3f51753
1a5f39c
b876765
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
package org.apache.skywalking.apm.plugin.kafka; | ||
|
||
import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
import org.apache.kafka.clients.consumer.ConsumerRecords; | ||
import org.apache.kafka.common.TopicPartition; | ||
|
||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
/** | ||
* kafak3.7 removed the method named pollForFetches with return type Fetch. | ||
* When the kafka version < 3.7, the enhance method could be: org.apache.kafka.clients.consumer.KafkaConsumer#pollForFetches. | ||
* But when the kafka version >= 3.7, the enhance method should be: org.apache.kafka.clients.consumer.KafkaConsumer#poll(java.time.Duration). | ||
* And the return type was also changed, from org.apache.kafka.clients.consumer.internals.Fetch to org.apache.kafka.clients.consumer.ConsumerRecords. | ||
*/ | ||
public class Kafka37ConsumerInterceptor extends KafkaConsumerInterceptor { | ||
|
||
@Override | ||
protected Map<TopicPartition, List<ConsumerRecord<?, ?>>> fetchRecords(Object retObj) { | ||
Map<TopicPartition, List<ConsumerRecord<?, ?>>> rsp = new HashMap<>(); | ||
if (retObj instanceof ConsumerRecords) { | ||
ConsumerRecords<?, ?> consumerRecords = (ConsumerRecords<?, ?>) retObj; | ||
if (consumerRecords.isEmpty()) { | ||
return rsp; | ||
} | ||
for (ConsumerRecord<?, ?> consumerRecord : consumerRecords) { | ||
int partition = consumerRecord.partition(); | ||
String topic = consumerRecord.topic(); | ||
TopicPartition topicPartition = new TopicPartition(topic, partition); | ||
List<ConsumerRecord<?, ?>> recordsForTp = rsp.computeIfAbsent(topicPartition, k -> new ArrayList<>()); | ||
recordsForTp.add(consumerRecord); | ||
} | ||
} | ||
return rsp; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
package org.apache.skywalking.apm.plugin.kafka.define; | ||
|
||
import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch; | ||
import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName; | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the comment, I think we can focus on the difference instead of repeating what is already written in the parent class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, the difference is the method named pollForFetchs was removed from KafkaConsumer to another two classes, so the original interceptor can not intercept it. Because of the enhance class is changed, so I create two new classes to repair the uncompatible problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I meant you can modify the comment for this class...... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I forget to change the comment |
||
* Here is the intercept process steps. | ||
* | ||
* <pre> | ||
* 1. Record the topic when the client invoke <code>subscribed</code> method | ||
* 2. Create the entry span when the client invoke the method <code>pollOnce</code>. | ||
* 3. Extract all the <code>Trace Context</code> by iterate all <code>ConsumerRecord</code> | ||
* 4. Stop the entry span when <code>pollOnce</code> method finished. | ||
* </pre> | ||
*/ | ||
public class Kafka37AsyncConsumerInstrumentation extends KafkaConsumerInstrumentation { | ||
|
||
private static final String ENHANCE_CLASS_37_ASYNC = "org.apache.kafka.clients.consumer.internals.AsyncKafkaConsumer"; | ||
|
||
@Override | ||
protected ClassMatch enhanceClass() { | ||
return byName(ENHANCE_CLASS_37_ASYNC); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
package org.apache.skywalking.apm.plugin.kafka.define; | ||
|
||
import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch; | ||
import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName; | ||
|
||
/** | ||
* Here is the intercept process steps. | ||
* | ||
* <pre> | ||
* 1. Record the topic when the client invoke <code>subscribed</code> method | ||
* 2. Create the entry span when the client invoke the method <code>pollOnce</code>. | ||
* 3. Extract all the <code>Trace Context</code> by iterate all <code>ConsumerRecord</code> | ||
* 4. Stop the entry span when <code>pollOnce</code> method finished. | ||
* </pre> | ||
*/ | ||
public class Kafka37LegacyConsumerInstrumentation extends KafkaConsumerInstrumentation { | ||
|
||
private static final String ENHANCE_CLASS_37_LEGACY = "org.apache.kafka.clients.consumer.internals.LegacyKafkaConsumer"; | ||
|
||
@Override | ||
protected ClassMatch enhanceClass() { | ||
return byName(ENHANCE_CLASS_37_LEGACY); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,103 +63,103 @@ public class KafkaConsumerInstrumentation extends AbstractKafkaInstrumentation { | |
@Override | ||
public ConstructorInterceptPoint[] getConstructorsInterceptPoints() { | ||
return new ConstructorInterceptPoint[] { | ||
new ConstructorInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getConstructorMatcher() { | ||
return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE); | ||
} | ||
new ConstructorInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getConstructorMatcher() { | ||
return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE); | ||
} | ||
|
||
@Override | ||
public String getConstructorInterceptor() { | ||
return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS; | ||
} | ||
}, | ||
new ConstructorInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getConstructorMatcher() { | ||
return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE); | ||
} | ||
|
||
@Override | ||
public String getConstructorInterceptor() { | ||
return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS; | ||
} | ||
}, | ||
@Override | ||
public String getConstructorInterceptor() { | ||
return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS; | ||
} | ||
}, | ||
new ConstructorInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getConstructorMatcher() { | ||
return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE); | ||
} | ||
|
||
@Override | ||
public String getConstructorInterceptor() { | ||
return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS; | ||
} | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert re-format. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have already recerted it, this class dosen't change, I have already rollback it to the original version There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, you didn't. Change is still there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Those changes are compare to the changes which are not correct I commit before... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I dont't know why.... I check out the original branch and copy the codes into the class file, but the IDE says that there is no difference..... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Resolved |
||
|
||
}; | ||
} | ||
|
||
@Override | ||
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() { | ||
return new InstanceMethodsInterceptPoint[] { | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
// targeting Kafka Client < 3.2 | ||
return named(ENHANCE_METHOD).or(named(ENHANCE_COMPATIBLE_METHOD).and(returns(Map.class))); | ||
} | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
// targeting Kafka Client < 3.2 | ||
return named(ENHANCE_METHOD).or(named(ENHANCE_COMPATIBLE_METHOD).and(returns(Map.class))); | ||
} | ||
|
||
@Override | ||
public String getMethodsInterceptor() { | ||
return INTERCEPTOR_CLASS; | ||
} | ||
@Override | ||
public String getMethodsInterceptor() { | ||
return INTERCEPTOR_CLASS; | ||
} | ||
|
||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
// targeting Kafka Client >= 3.2 | ||
return named(ENHANCE_COMPATIBLE_METHOD).and(returns(named("org.apache.kafka.clients.consumer.internals.Fetch"))); | ||
} | ||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
// targeting Kafka Client >= 3.2 | ||
return named(ENHANCE_COMPATIBLE_METHOD).and(returns(named("org.apache.kafka.clients.consumer.internals.Fetch"))); | ||
} | ||
|
||
@Override | ||
public String getMethodsInterceptor() { | ||
return INTERCEPTOR_CLASS_KAFKA3_2; | ||
} | ||
@Override | ||
public String getMethodsInterceptor() { | ||
return INTERCEPTOR_CLASS_KAFKA3_2; | ||
} | ||
|
||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
return named(SUBSCRIBE_METHOD) | ||
.and(takesArgumentWithType(0, SUBSCRIBE_INTERCEPT_TYPE_NAME)); | ||
} | ||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
return named(SUBSCRIBE_METHOD) | ||
.and(takesArgumentWithType(0, SUBSCRIBE_INTERCEPT_TYPE_NAME)); | ||
} | ||
|
||
@Override | ||
public String getMethodsInterceptor() { | ||
return SUBSCRIBE_INTERCEPT_CLASS; | ||
} | ||
@Override | ||
public String getMethodsInterceptor() { | ||
return SUBSCRIBE_INTERCEPT_CLASS; | ||
} | ||
|
||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
return named(SUBSCRIBE_METHOD) | ||
.and(takesArgumentWithType(0, SUBSCRIBE_INTERCEPT_TYPE_PATTERN)); | ||
} | ||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
return named(SUBSCRIBE_METHOD) | ||
.and(takesArgumentWithType(0, SUBSCRIBE_INTERCEPT_TYPE_PATTERN)); | ||
} | ||
|
||
@Override | ||
public String getMethodsInterceptor() { | ||
return SUBSCRIBE_INTERCEPT_CLASS; | ||
} | ||
@Override | ||
public String getMethodsInterceptor() { | ||
return SUBSCRIBE_INTERCEPT_CLASS; | ||
} | ||
|
||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
@Override | ||
public boolean isOverrideArgs() { | ||
return false; | ||
} | ||
}, | ||
new InstanceMethodsInterceptPoint() { | ||
@Override | ||
public ElementMatcher<MethodDescription> getMethodsMatcher() { | ||
|
@@ -184,4 +184,4 @@ public boolean isOverrideArgs() { | |
protected ClassMatch enhanceClass() { | ||
return byName(ENHANCE_CLASS); | ||
} | ||
} | ||
} |
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.
Why
instanceof
? Is there another case to have another parameter? The parent class hasMap<TopicPartition, List<ConsumerRecord<?, ?>>>
type as the parameter, but it should not affect this new Kafka37ConsumerInterceptor, right?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.
This class should be removed, I missed it, I'll delete it at next commit