Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

feat: add reporter plugins: kafka sarama #33

Merged
merged 23 commits into from
Oct 10, 2021

Conversation

matianjun1
Copy link
Contributor

add kafka reporter just like SkyWalking Python Agent implement

e2e test cant do because of thats for grpc

@wu-sheng
Copy link
Member

You can't do through mock collector, but we have e2e framework for this.

An example in Java repo, https://github.com/apache/skywalking-java/blob/main/.github/workflows/e2e.yaml.
The only missing is that, it doesn't deploy Kafka and activate plugin and receiver, which is easy through docker compose.

@wu-sheng wu-sheng added this to the 1.3.0 milestone Sep 23, 2021
@wu-sheng wu-sheng added the enhancement New feature or request label Sep 23, 2021
@arugal
Copy link
Member

arugal commented Sep 23, 2021

image

@matianjun1 Please add license header

sarama/go.mod Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

How about use kafka-reporter rather than kakfareporter?

@matianjun1
Copy link
Contributor Author

How about use kafka-reporter rather than kakfareporter?

For golang package names. Good package names are short and clear. They are lower case, with no under_scores or mixedCaps.

I agree with using kafka-reporter rather than kakfareporter is better in other languages.

https://go.dev/blog/package-names

@wu-sheng
Copy link
Member

How about /reporter/kafka as the root folder?

@matianjun1
Copy link
Contributor Author

@wu-sheng @arugal

Need some help with go_kafka_reporter_plugin_test.yaml because of the e2e failed.

@arugal
Copy link
Member

arugal commented Sep 27, 2021

@wu-sheng @arugal

Need some help with go_kafka_reporter_plugin_test.yaml because of the e2e failed.

@matianjun1 You need to activate kafka-fetcher.

@matianjun1
Copy link
Contributor Author

@arugal
I have set the SW_KAFKA_FETCHER as default.

And using kafka-console-consumer.sh --bootstrap-server=localhost:9092 --topic=skywalking-managements I can see the messages in skywalking-managements.

But the oap server still not receive trace messages.

@arugal
Copy link
Member

arugal commented Sep 29, 2021

@matianjun1 The e2e failed because the service was not found, please see here #33 (comment)

@matianjun1
Copy link
Contributor Author

@arugal
Copy link
Member

arugal commented Sep 29, 2021

@arugal
Copy link
Member

arugal commented Sep 29, 2021

image

@matianjun1

Copy link
Member

@arugal arugal left a comment

Choose a reason for hiding this comment

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

replace range with contains

kafkareporter/test/expected/service-instance.yml Outdated Show resolved Hide resolved
kafkareporter/test/expected/trace-info-detail.yml Outdated Show resolved Hide resolved
Copy link
Member

@arugal arugal left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng merged commit ee26026 into SkyAPM:master Oct 10, 2021
@wu-sheng
Copy link
Member

@matianjun1 thank you, this is about your contribution. https://twitter.com/ASFSkyWalking/status/1447230491128582150

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants