Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng merged PR #178: URL: https://github.com/apache/skywalking-go/pull/178 -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2024266270 Take your time, it is urgent. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2024247875 > all thing, it is not very useful in agent mode. We have specif Sorry, I get your point now! I will update the scenario later. My network problem finally solved by using vpn. Otherwise Docker images from google can not be downloaded, and downloading go dependents from github in container will failled. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2024220395 You still miss this, > The existing one is at here, https://github.com/apache/skywalking-go/tree/main/test/plugins/scenarios/gin You should enhance it, verify locally, and update. We need this feature verified in this demo application instrumentation. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2024218174 All modification and test is done, any things else do I need to do? -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2017054187 There is nothing we can do about your network environment. And it is common for go dev, we don't add anything new. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2017041463 > UT is just a small thing, it is not very useful in agent mode. We have specific plugin test, https://skywalking.apache.org/docs/skywalking-go/next/en/development-and-contribution/write-plugin-testing/ > > The existing one is at here, https://github.com/apache/skywalking-go/tree/main/test/plugins/scenarios/gin > > You should enhance it, verify locally, and update. The wall is making me crazy... -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1533350878
##
plugins/gin/config.go:
##
@@ -0,0 +1,23 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gin
+
+//skywalking:config gin
+var config struct {
+ CollectHttpHeaders string `config:"collect_http_headers"`
Review Comment:
Working on it.
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
mrproliu commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1532188256
##
plugins/gin/config.go:
##
@@ -0,0 +1,23 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gin
+
+//skywalking:config gin
+var config struct {
+ CollectHttpHeaders string `config:"collect_http_headers"`
Review Comment:
You need to follow these steps:
1. Adding the `[]string` case in there:
https://github.com/apache/skywalking-go/blob/main/tools/go-agent/instrument/plugins/enhance_config.go#L219-L240
2. Adding `ParseStringArray` into the [ToolOperator
interface](https://github.com/apache/skywalking-go/blob/main/plugins/core/operator/tools.go)
and [implement
it](https://github.com/apache/skywalking-go/blob/main/plugins/core/tracer_tools.go).
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1532142589
##
plugins/gin/config.go:
##
@@ -0,0 +1,23 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gin
+
+//skywalking:config gin
+var config struct {
+ CollectHttpHeaders string `config:"collect_http_headers"`
Review Comment:
Yes, that is bothering me. And I want to solve that. where should I getting
start?
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009657536 > > UT is just a small thing, it is not very useful in agent mode. We have specific plugin test, https://skywalking.apache.org/docs/skywalking-go/next/en/development-and-contribution/write-plugin-testing/ > > The existing one is at here, https://github.com/apache/skywalking-go/tree/main/test/plugins/scenarios/gin > > You should enhance it, verify locally, and update. > > UT is no doubt necessary. Yes. But it is not verifying the real case. It only verified expected parameter works. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009647371 > @IceSoda177 hello bro, could you help to update the docs/en/agent/plugin-configurations.md? And plz fix the CI. Will do -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009645456 > UT is just a small thing, it is not very useful in agent mode. We have specific plugin test, https://skywalking.apache.org/docs/skywalking-go/next/en/development-and-contribution/write-plugin-testing/ > > The existing one is at here, https://github.com/apache/skywalking-go/tree/main/test/plugins/scenarios/gin > > You should enhance it, verify locally, and update. UT is no doubt necessary. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
mrproliu commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531891550
##
plugins/gin/config.go:
##
@@ -0,0 +1,23 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gin
+
+//skywalking:config gin
+var config struct {
+ CollectHttpHeaders string `config:"collect_http_headers"`
Review Comment:
A small suggestion, could you change his data type to `[]string`? Although
it may be a bit challenging, you might need to provide parsing capabilities in
the agent core, so that you don't have to use `runtimeConfig` and reprocess it
for each request. If it's still difficult for you, I can help with it. :)
##
plugins/gin/config.go:
##
@@ -0,0 +1,23 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package gin
+
+//skywalking:config gin
+var config struct {
+ CollectHttpHeaders string `config:"collect_http_headers"`
Review Comment:
A small suggestion, could you change this data type to `[]string`? Although
it may be a bit challenging, you might need to provide parsing capabilities in
the agent core, so that you don't have to use `runtimeConfig` and reprocess it
for each request. If it's still difficult for you, I can help with it. :)
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009147085 UT is just a small thing, it is not very useful in agent mode. We have specific plugin test, https://skywalking.apache.org/docs/skywalking-go/next/en/development-and-contribution/write-plugin-testing/ The existing one is at here, https://github.com/apache/skywalking-go/tree/main/test/plugins/scenarios/gin You should enhance it, verify locally, and update. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
CodePrometheus commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009268928 > @CodePrometheus Would you like to review this? I am more than willing to participate in code review, and I will pay attention during my free time at work~ (Beijing time) -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
CodePrometheus commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009293900 @IceSoda177 hello bro, could you help to update the docs/en/agent/plugin-configurations.md? And plz fix the CI. -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
mrproliu commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531677744
##
tools/go-agent/config/agent.default.yaml:
##
@@ -97,3 +97,5 @@ plugin:
redis:
# Limit the bytes size of redis args request
max_args_bytes: ${SW_AGENT_PLUGIN_CONFIG_REDIS_MAX_ARGS_BYTES:1024}
+gin:
+ collect_http_headers: ${SW_AGENT_PLUGIN_CONFIG_GIN_COLLECT_HTTP_HEADERS:}
Review Comment:
And the default value should be `false` 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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
mrproliu commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531677744
##
tools/go-agent/config/agent.default.yaml:
##
@@ -97,3 +97,5 @@ plugin:
redis:
# Limit the bytes size of redis args request
max_args_bytes: ${SW_AGENT_PLUGIN_CONFIG_REDIS_MAX_ARGS_BYTES:1024}
+gin:
+ collect_http_headers: ${SW_AGENT_PLUGIN_CONFIG_GIN_COLLECT_HTTP_HEADERS:}
Review Comment:
And the default value should be `false` 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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531677876
##
tools/go-agent/config/agent.default.yaml:
##
@@ -97,3 +97,5 @@ plugin:
redis:
# Limit the bytes size of redis args request
max_args_bytes: ${SW_AGENT_PLUGIN_CONFIG_REDIS_MAX_ARGS_BYTES:1024}
+gin:
+ collect_http_headers: ${SW_AGENT_PLUGIN_CONFIG_GIN_COLLECT_HTTP_HEADERS:}
Review Comment:
Yes, config.go is missing
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
mrproliu commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531675819
##
tools/go-agent/config/agent.default.yaml:
##
@@ -97,3 +97,5 @@ plugin:
redis:
# Limit the bytes size of redis args request
max_args_bytes: ${SW_AGENT_PLUGIN_CONFIG_REDIS_MAX_ARGS_BYTES:1024}
+gin:
+ collect_http_headers: ${SW_AGENT_PLUGIN_CONFIG_GIN_COLLECT_HTTP_HEADERS:}
Review Comment:
I think you missing the configuration declaration. please take a look at
other plugins, such as
https://github.com/apache/skywalking-go/blob/main/plugins/go-redisv9/config.go
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531673522
##
plugins/gin/intercepter.go:
##
@@ -70,3 +77,26 @@ func isFirstHandle(c interface{}) bool {
}
return true
}
+
+func collectHttpHeaders(span tracing.Span, requestHeaders http.Header) {
+ if len(runtimeConfig.CollectHttpHeaders) == 0 {
Review Comment:
> You should update the gin plugin test to verify this feature.
right
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531672316
##
plugins/gin/intercepter.go:
##
@@ -70,3 +77,26 @@ func isFirstHandle(c interface{}) bool {
}
return true
}
+
+func collectHttpHeaders(span tracing.Span, requestHeaders http.Header) {
+ if len(runtimeConfig.CollectHttpHeaders) == 0 {
Review Comment:
> If len==0 then split? Do you mis-write this?
They are different config. I use runtimeConfig to store header list, cause
the config can't accept array type config param.
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on PR #178: URL: https://github.com/apache/skywalking-go/pull/178#issuecomment-2009007431 @CodePrometheus Would you like to review this? -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531659046
##
plugins/gin/intercepter.go:
##
@@ -70,3 +77,26 @@ func isFirstHandle(c interface{}) bool {
}
return true
}
+
+func collectHttpHeaders(span tracing.Span, requestHeaders http.Header) {
+ if len(runtimeConfig.CollectHttpHeaders) == 0 {
Review Comment:
You should update the gin plugin test to verify this feature.
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Supporting http headers collection for Gin [skywalking-go]
wu-sheng commented on code in PR #178:
URL: https://github.com/apache/skywalking-go/pull/178#discussion_r1531658612
##
plugins/gin/intercepter.go:
##
@@ -70,3 +77,26 @@ func isFirstHandle(c interface{}) bool {
}
return true
}
+
+func collectHttpHeaders(span tracing.Span, requestHeaders http.Header) {
+ if len(runtimeConfig.CollectHttpHeaders) == 0 {
Review Comment:
If len==0 then split? Do you mis-write this?
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] Supporting http headers collection for Gin [skywalking-go]
IceSoda177 opened a new pull request, #178: URL: https://github.com/apache/skywalking-go/pull/178 Supporting http headers collection for Gin -- 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-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
