Skip to content

Conversation

@zJiaJun
Copy link
Contributor

@zJiaJun zJiaJun commented Apr 23, 2025

Purpose

This PR replaces the custom emptyReporter implementation in plugins/core/tracer.go with the existing reporter.NewDiscardReporter() to reduce code duplication and improve maintainability.

Changes

  • Replace &emptyReporter{} with reporter.NewDiscardReporter() in the newTracer() method
  • Remove the redundant emptyReporter struct and its method implementations

Motivation and Context

The emptyReporter and DiscardReporter have identical functionality - they both implement the Reporter interface but don't perform any actual operations. Using the existing DiscardReporter implementation reduces code duplication and follows the DRY principle.

@wu-sheng
Copy link
Member

@mrproliu This seems to be a code level optimization only. We could keep this in 0.7?

@wu-sheng wu-sheng added this to the 0.7.0 milestone Apr 23, 2025
@wu-sheng wu-sheng requested review from Copilot and mrproliu April 23, 2025 14:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the tracer functionality to reduce code duplication by replacing the custom emptyReporter with the existing DiscardReporter.

  • Replaces the instantiation of &emptyReporter{} with reporter.NewDiscardReporter() in newTracer.
  • Removes the emptyReporter type and its methods.
  • Updates discard_reporter.go to return ConnectionStatusDisconnect for clarity.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
plugins/core/tracer.go Replaces emptyReporter with DiscardReporter and removes unused code.
plugins/core/reporter/discard_reporter.go Updates ConnectionStatus to return ConnectionStatusDisconnect.

@zJiaJun
Copy link
Contributor Author

zJiaJun commented Apr 23, 2025

@mrproliu This seems to be a code level optimization only. We could keep this in 0.7?

@wu-sheng
I would like to propose adding a Kafka Reporter implementation to SkyWalking-Go to enhance its data reporting capabilities. This feature would allow users to send tracing data to Apache SkyWalking OAP server through Kafka, which is particularly beneficial in high-throughput environments

I would be happy to contribute to this implementation if the proposal is accepted. Please let me know your thoughts on this addition and any specific requirements or considerations I should be aware of.

@wu-sheng
Copy link
Member

Kafka reporter is fine, as other agents have that.
The key is dependency management. We need to avoid impacting user app dependency tree.

mrproliu
mrproliu previously approved these changes Apr 24, 2025
@mrproliu mrproliu added the enhancement New feature or request label Apr 24, 2025
@wu-sheng
Copy link
Member

Kafka reporter is fine, as other agents have that.
The key is dependency management. We need to avoid impacting user app dependency tree.

It seems we don't have a way to avoid that.
Please go ahead, and let us pick latest kafka go client for this new feature.

@mrproliu
Copy link
Contributor

@zJiaJun Hi, please fix the UT.

@zJiaJun
Copy link
Contributor Author

zJiaJun commented Apr 24, 2025

@zJiaJun Hi, please fix the UT.

Fixed

@zJiaJun
Copy link
Contributor Author

zJiaJun commented Apr 24, 2025

It seems we don't have a way to avoid that.
Please go ahead, and let us pick latest kafka go client for this new feature.

I'll start working on the Kafka Reporter implementation. It may take some time, and I plan to use segmentio/kafka-go v0.4.47 as the client library.

@wu-sheng wu-sheng closed this Apr 24, 2025
@wu-sheng wu-sheng reopened this Apr 24, 2025
@wu-sheng
Copy link
Member

It seems we don't have a way to avoid that.
Please go ahead, and let us pick latest kafka go client for this new feature.

I'll start working on the Kafka Reporter implementation. It may take some time, and I plan to use segmentio/kafka-go v0.4.47 as the client library.

Take your time. We have tagged 0.6 release. We have plenty time for 0.7.

@mrproliu mrproliu merged commit 14cdc63 into apache:main Apr 24, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants