You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2023/07/20 13:51:24 UTC
[GitHub] [skywalking-go] mrproliu opened a new pull request, #81: Support enhance native `mutex` as plugin
mrproliu opened a new pull request, #81:
URL: https://github.com/apache/skywalking-go/pull/81
(no comment)
--
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
[GitHub] [skywalking-go] mrproliu commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646603514
I mean its only could calculate the metrics when have a span(tracing context) in current goroutine.
--
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
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270227129
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Yes.
acquisition latency: getting lock used time.
using duration: the duration between lock and unlock time.
error count: can't getting lock count(for `TryLock`).
Am I 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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-go] wu-sheng commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646593215
Isn't lock having a name?
--
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
[GitHub] [skywalking-go] wu-sheng commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646596504
Then, how to distict the different lockers? I am feeling aggregating globally is not that useful.
--
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
[GitHub] [skywalking-go] jcchavezs commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "jcchavezs (via GitHub)" <gi...@apache.org>.
jcchavezs commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1271534143
##########
tools/go-agent/config/loader.go:
##########
@@ -282,6 +282,20 @@ func (s *StringValue) GetListStringResult() []string {
return strings.Split(val, ",")
}
+// GetListStringResultByLookup get the value from os.LookupEnv, if the env found, even it's empty
+func (s *StringValue) GetListStringResultByLookup() []string {
Review Comment:
Shall we test this function? I know it is very simple but I can't find a good reason for not to test 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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1271300740
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
The metrics have been added to this PR.
--
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
[GitHub] [skywalking-go] wu-sheng commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270220105
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Lock begin method should indicate the pending time before lock acquired, right?
At least 3 metrics, acquisition latency, using duration, error count(for try lock)
--
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
[GitHub] [skywalking-go] wu-sheng commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270233323
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Yes, would this be listed as Golang VM tab?
--
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
[GitHub] [skywalking-go] wu-sheng commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646602048
What do you nean in tracing context? What is IN?
--
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
[GitHub] [skywalking-go] mrproliu commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646595223
In golang code, it's just under the "sync" package, there is no name.
--
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
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270196388
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Yes, I think the lock time is the main metric for measuring a lock performance. We can intercept the lock begin time and finish time.
Also, If it's using `TryLock` we can measure the failure lock count.
--
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
[GitHub] [skywalking-go] mrproliu commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646598258
Yes, that's why I say it maybe difficult to understand. For now, I just calculating the metrics that must in a tracing context.
Other solution is we only calculate the lock performance when executing specific method signature(user defined), but it maybe take more CPU resource.
--
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
[GitHub] [skywalking-go] wu-sheng commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646605339
I noticed that, but if there are more than one lock involved, it will fail, 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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-go] wu-sheng commented on pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#issuecomment-1646854232
This concers me, as the instrument affects the agent kernel.
Let's hold it for a little longer, to see whether we have a better solution.
--
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
[GitHub] [skywalking-go] wu-sheng commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270194849
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
I mean, how to measure a lock performance? Usually, if lock pending time is long(lock method is blocked), it mean too many competition or not enough resources pooled.
--
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
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270234529
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Yes, I could add to 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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1271300716
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
The metrics been added to this PR.
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
The metrics been added to this PR.
--
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
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1271300716
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
The metrics been added to this PR.
--
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
[GitHub] [skywalking-go] mrproliu commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270193598
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
> Lock client? Is it a distributed lock solution as well?
It's a local lock solution. Not support distribution.
> Also, you only support tracing, but not logic endpoint or metric of lock(pending, release, etc.).
> Should we consider that?
I think the metrics should be considered, but the original lock would be triggered by Go Self and frameworks. The metrics maybe difficult to understand.
--
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
[GitHub] [skywalking-go] mrproliu closed pull request #81: Support enhance native `mutex` as plugin
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu closed pull request #81: Support enhance native `mutex` as plugin
URL: https://github.com/apache/skywalking-go/pull/81
--
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
[GitHub] [skywalking-go] wu-sheng commented on a diff in pull request #81: Support enhance native `mutex` as plugin
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #81:
URL: https://github.com/apache/skywalking-go/pull/81#discussion_r1270188085
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
Also, you only support tracing, but not logic endpoint or metric of lock(pending, release, etc.).
Should we consider that?
##########
CHANGES.md:
##########
@@ -22,6 +22,7 @@ Release Notes.
* Support [Native SQL](https://pkg.go.dev/net/http) database client framework with [MySQL Driver](github.com/go-sql-driver/mysql).
* Support [Logrus](https://github.com/sirupsen/logrus) log report to the backend.
* Support [Zap](https://github.com/uber-go/zap) log report to the backend.
+* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
Review Comment:
```suggestion
* Support [Native Mutex](https://pkg.go.dev/sync) lock client framework.
```
Lock client? Is it a distributed lock solution as well?
--
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