You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "StoneMoe (via GitHub)" <gi...@apache.org> on 2024/12/24 11:11:04 UTC
[PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
StoneMoe opened a new pull request, #213:
URL: https://github.com/apache/skywalking-go/pull/213
As we discussed in Slack, this will fix http requests with empty `request.Host`, such as elasticsearch client, which set request URL after a path-only request creation.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2561020797
Reference to the slack link: https://the-asf.slack.com/archives/CHQ1L4T54/p1734924623039789
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2561022038
<img width="220" alt="image" src="https://github.com/user-attachments/assets/8fbd5d29-f2c0-4df2-9e82-35a7fe918918" />
Binary is not allowed. Please remove this unexpected file.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2562245567
Can you help to check why there say `2 occurrences`, If so, we should add a new const here.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#discussion_r1896671833
##########
plugins/http/client_intercepter.go:
##########
@@ -30,12 +30,18 @@ type ClientInterceptor struct {
func (h *ClientInterceptor) BeforeInvoke(invocation operator.Invocation) error {
request := invocation.Args()[0].(*http.Request)
- s, err := tracing.CreateExitSpan(fmt.Sprintf("%s:%s", request.Method, request.URL.Path), request.Host, func(headerKey, headerValue string) error {
+ host := request.Host
+ if host == "" {
+ if request.URL != nil {
Review Comment:
```suggestion
if host == "" && request.URL != nil {
```
I think it can be simplified to a single line.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu merged PR #213:
URL: https://github.com/apache/skywalking-go/pull/213
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2562199140
@StoneMoe, please fix the CI failure.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2561019915
Please update the [CHANGES.md](https://github.com/apache/skywalking-go/blob/main/CHANGES.md) file.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "StoneMoe (via GitHub)" <gi...@apache.org>.
StoneMoe commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2562276713
> Can you help to check why there say `2 occurrences`, If so, we should add a new const here.
In test file there is a new "http" string I added.
Considering change goconst option `ignore-tests` to `true` instead I think.
--
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
Re: [PR] Use request.URL.Host instead when request.Host is empty [skywalking-go]
Posted by "StoneMoe (via GitHub)" <gi...@apache.org>.
StoneMoe commented on PR #213:
URL: https://github.com/apache/skywalking-go/pull/213#issuecomment-2562234609
> @StoneMoe, please fix the CI failure.
It says:
```
Error: instrument.go:38:9: string `http` has 2 occurrences, make it a constant (goconst)
return "http"
^
```
Maybe we should fix it by incresing goconst `min-occurrences` options? @mrproliu
--
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