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

feat: add dubbo-go plugins #34

Merged
merged 45 commits into from
Oct 11, 2021
Merged

feat: add dubbo-go plugins #34

merged 45 commits into from
Oct 11, 2021

Conversation

royal-dargon
Copy link
Contributor

Add dubbo-go plugins.
Add test files.

@arugal arugal self-requested a review September 30, 2021 04:46
@arugal arugal added the plugin label Sep 30, 2021
@arugal arugal added this to the 1.3.0 milestone Sep 30, 2021
dubbo-go/dubbo-go.go Outdated Show resolved Hide resolved
.github/workflows/plugin_test.yaml Outdated Show resolved Hide resolved
dubbo-go/dubbo-go.go Outdated Show resolved Hide resolved
log.Fatalf("set tracer error: %v \n", err)
}
// set extra tags and report tags
dubbo_go.SetClientExtraTags("extra-tags", "server")
Copy link
Member

Choose a reason for hiding this comment

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

@royal-dargon The server should use SetServerExtraTags and SetServerReportTags, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@arugal
Copy link
Member

arugal commented Oct 1, 2021

@royal-dargon Please add dubbo-go here.

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.

@royal-dargon Basically no problem, please commit suggestions, and set PR to ready review.

dubbo-go/README.md Outdated Show resolved Hide resolved
dubbo-go/README.md Outdated Show resolved Hide resolved
dubbo-go/dubbo-go.go Outdated Show resolved Hide resolved
@arugal
Copy link
Member

arugal commented Oct 1, 2021

After PR merged, the component id and Component-Server-Mappings of dubbo-go need to be added in component-libraries.yml.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 2, 2021

After PR merged, the component id and Component-Server-Mappings of dubbo-go need to be added in component-libraries.yml.

As dubbo-go belongs to dubbo, how about using Dubbo's component id?

@royal-dargon royal-dargon marked this pull request as ready for review October 2, 2021 02:54
Comment on lines 19 to 20
{{- range .segmentItems }}
{{- if eq .serviceName "dubbo-go-server" }}
Copy link
Member

Choose a reason for hiding this comment

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

Notice, take a look at this context, apache/skywalking#7849 (comment).

range should not be used in the expected file.

@arugal You also should update the existing cases.

Comment on lines 34 to 35
componentIDGo2SkyClient = 5013
componentIDGo2SkyServer = 5014
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at this, https://github.com/apache/skywalking/blob/ebd040eb1a3ed92e79da6f917cb9c9fe44a6938d/oap-server/server-starter/src/main/resources/component-libraries.yml#L41-L43.

You don't need client and server, also, you don't need new component IDs. You could update languages field in the YAML of main repo.

Copy link
Member

Choose a reason for hiding this comment

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

Mapping is for indicating a server side which is not tracing. Such as MySQL JDBC driver to MySQL server. For MySQL server, we don't expect an agent installed.

dubbo-go/test/client/cmd/client.go Outdated Show resolved Hide resolved
dubbo-go/test/docker-compose.yml Outdated Show resolved Hide resolved
dubbo-go/test/dubbo_go_plugin_test.yaml Outdated Show resolved Hide resolved
dubbo-go/test/expected.data.yml Outdated Show resolved Hide resolved
dubbo-go/dubbo-go.go Outdated Show resolved Hide resolved
dubbo-go/dubbo-go.go 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 6562b6c into SkyAPM:master Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants