You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/10/18 14:59:42 UTC
[GitHub] [skywalking-python] jiang1997 opened a new pull request, #243: feat: add Kafka support to MeterReportService
jiang1997 opened a new pull request, #243:
URL: https://github.com/apache/skywalking-python/pull/243
<!-- Uncomment the following checklist WHEN AND ONLY WHEN you're adding a new plugin -->
<!--
- [ ] Add a test case for the new plugin
- [ ] Add a CHANGELOG entry for the new plugin
- [ ] Add a component id in [the main repo](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-booster-ui/tree/main/src/assets/img/technologies)
- [ ] Rebuild the `requirements.txt` by running `tools/env/build_requirements_(linux|windows).sh`
- [ ] Rebuild the `Plugins.md` documentation by running `make doc-gen`
-->
https://github.com/apache/skywalking-python/pull/238
Thanks to @Superskyyy's assistance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] wu-sheng commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1287091847
@Superskyyy Are we going to have a 1.0.0 release shortly? I noticed there are only 2 left on https://github.com/apache/skywalking/milestone/140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1283004990
LGTM and congratulations on finishing all parts of the OSPP task! I appreciate your contribution and look forward to more haha.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it faild to pass the tests.
After some searching, I found that the default encoding of `org.apache.kafka.common.serialization.StringDeserializer` is `UTF-8` and the default string econding of python3 is `UTF-16` or `UTF-32`. So the conversion is needed.
https://kafka.apache.org/0100/javadoc/index.html?org/apache/kafka/common/serialization/StringDeserializer.html
https://github.com/apache/skywalking/blob/b5cd3fad7e21782a254b83d1fa67cdc14c391baf/oap-server/server-fetcher-plugin/kafka-fetcher-plugin/src/main/java/org/apache/skywalking/oap/server/analyzer/agent/kafka/KafkaFetcherHandlerRegister.java#L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r1001340733
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
You are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998718811
##########
tests/e2e/case/kafka/e2e.yaml:
##########
@@ -64,8 +64,8 @@ verify:
# TODO: Metric Collection Implementation is not merged https://github.com/apache/skywalking/issues/7084
Review Comment:
remove these
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it failed to pass the tests.
Say here(https://github.com/dpkp/kafka-python/blob/4d598055dab7da99e41bfcceffa8462b32931cdd/kafka/producer/kafka.py#L555) says that `key` as an argument of `KafkaProducer.send` must be type bytes. So the conversion is needed (`config.service_instance.encode()` should work too).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1287044984
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1287672170
> @Superskyyy Are we going to have a 1.0.0 release shortly? I noticed there are only 2 left on [apache/skywalking/milestone/140](https://github.com/apache/skywalking/milestone/140)
I think so, the preforking behavior relates to somewhat weird issues in upstream dependencies that cannot be solved easily. I will try to take another look soon and go for 1.0.0, regardless if it's resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it faild to pass the tests.
After some searching, I found that the default encoding of `org.apache.kafka.common.serialization.StringDeserializer` is `UTF-8` and the default string econding of python3 is `UTF-16` or `UTF-8`. So the conversion is needed.
https://github.com/apache/skywalking/blob/b5cd3fad7e21782a254b83d1fa67cdc14c391baf/oap-server/server-fetcher-plugin/kafka-fetcher-plugin/src/main/java/org/apache/skywalking/oap/server/analyzer/agent/kafka/KafkaFetcherHandlerRegister.java#L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r1001200664
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
Actually I meant the value part, the key part is still needed. Sorry didn't make myself clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998718283
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
Actually, this bytes() conversion is not necessary. Can you take a look if it still works without casting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy merged pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy merged PR #243:
URL: https://github.com/apache/skywalking-python/pull/243
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it faild to pass the tests.
After some searching, I found that the default encoding of `org.apache.kafka.common.serialization.StringDeserializer` is `UTF-8` and the default string econding of python3 is `UTF-16` or `UTF-8`. So the conversion is needed.
https://kafka.apache.org/0100/javadoc/index.html?org/apache/kafka/common/serialization/StringDeserializer.html
https://github.com/apache/skywalking/blob/b5cd3fad7e21782a254b83d1fa67cdc14c391baf/oap-server/server-fetcher-plugin/kafka-fetcher-plugin/src/main/java/org/apache/skywalking/oap/server/analyzer/agent/kafka/KafkaFetcherHandlerRegister.java#L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998992203
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I tried and it failed to pass tests.
After some searching, I found that the default string encoding of Java is `UTF-8` and the default string encoding of python3 is `UTF-16` or `UTF-32`. So the conversion is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it failed to pass the tests.
[Here](https://github.com/dpkp/kafka-python/blob/4d598055dab7da99e41bfcceffa8462b32931cdd/kafka/producer/kafka.py#L555) says that `key` as an argument of `KafkaProducer.send` must be type bytes. So the conversion is needed (`config.service_instance.encode()` should work too).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1283592573
> LGTM and congratulations on finishing all parts of the OSPP task! I appreciate your contribution and look forward to more haha.
I'm also looking forward to attending aiops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1283592176
>
I'm also looking forward to attending aiops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it failed to pass the tests.
Say [here](https://github.com/dpkp/kafka-python/blob/4d598055dab7da99e41bfcceffa8462b32931cdd/kafka/producer/kafka.py#L555) that `key` as an argument of `KafkaProducer.send` must be type bytes. So the conversion is needed (`config.service_instance.encode()` should work too).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] jiang1997 commented on a diff in pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
jiang1997 commented on code in PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#discussion_r998998879
##########
skywalking/client/kafka.py:
##########
@@ -127,6 +128,19 @@ def report(self, generator):
self.producer.send(topic=self.topic, key=key, value=value)
+class KafkaMeterDataReportService(MeterReportService):
+ def __init__(self):
+ self.producer = KafkaProducer(**kafka_configs)
+ self.topic = config.kafka_topic_meter
+
+ def report(self, generator):
+ collection = MeterDataCollection()
+ collection.meterData.extend(list(generator))
+ key = bytes(config.service_instance, encoding='utf-8')
+ value = bytes(collection.SerializeToString())
Review Comment:
I have tried and it faild to pass the tests.
After some searching, I found that the default encoding of `org.apache.kafka.common.serialization.StringDeserializer` is `UTF-8` and the default string econding of python3 is `UTF-16` or `UTF-8`. So the conversion is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] Superskyyy commented on pull request #243: feat: add Kafka support to MeterReportService
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on PR #243:
URL: https://github.com/apache/skywalking-python/pull/243#issuecomment-1283012308
Just some minor stuff, others are good. ^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org