Support echo instrument #74
Conversation
There was a problem hiding this comment.
Please fix the comments and update the plugin documentation and CHANGES.md.
| @@ -6,16 +6,17 @@ use ( | |||
| // define the plugins | |||
| ./plugins/core | |||
| ./plugins/dubbo | |||
| ./plugins/echov4 | |||
There was a problem hiding this comment.
Please keep the original style.
plugins/echov4/go.mod
Outdated
| go 1.20 | ||
|
|
||
| require ( | ||
| github.com/apache/skywalking-go/plugins/core v0.0.0-20230712143836-5d2a80f0e1f5 |
There was a problem hiding this comment.
We already using the go workspace, so this module no needs to be required.
plugins/echov4/interceptor.go
Outdated
| if len(result) != 1 { | ||
| return fmt.Errorf("echo.New :skywalking return result length not equal 1") | ||
| } |
There was a problem hiding this comment.
You have already defined this method as one result, so this judgment is not necessary.
plugins/echov4/interceptor.go
Outdated
| tracing.WithLayer(tracing.SpanLayerHTTP), | ||
| tracing.WithTag(tracing.TagHTTPMethod, request.Method), | ||
| tracing.WithTag(tracing.TagURL, request.Host+request.URL.Path), | ||
| tracing.WithComponent(5006)) |
There was a problem hiding this comment.
The component ID should be updated?
There was a problem hiding this comment.
Ok, I'll register a unique component ID with the master library in another PR
There was a problem hiding this comment.
The PR has been merged. Please keep fixing other issues.
go.work
Outdated
| @@ -6,16 +6,17 @@ use ( | |||
| // define the plugins | |||
| ./plugins/core | |||
| ./plugins/dubbo | |||
| ./plugins/echo | |||
There was a problem hiding this comment.
Why did you change to the echo otherwise echov4?
There was a problem hiding this comment.
I'm trying to keep it consistent with the original library. I will change this to echov4.
|
Could you provide a correct link to echo framework? Is it a rpc or something? |
It is a web framework, see https://github.com/labstack/echo |
|
It is like a MIT licensed lib, do you need an icon for it on the topo? If so, please send a PR for that as well, otherwise, by default, it would be an earth icon only for server side. |
|
@ethan256 Any update? |
Sorry, I'm not familiar with certificates here and didn't understand what I needed to do. |
|
You are instrumenting a server lib, so generally, you should add the project logo to our website. |
|
Also, please resolve the conflict and conversations. |
|
Close for now, it seems contributor has no interest to follow reviewers comments. |
support echo instrumentation