Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metrics server #14

Merged
merged 31 commits into from
Mar 23, 2023
Merged

Add metrics server #14

merged 31 commits into from
Mar 23, 2023

Conversation

pddg
Copy link
Contributor

@pddg pddg commented Mar 3, 2023

Close #11

See #13 to get more detail.

@ymmt2005
Copy link
Member

ymmt2005 commented Mar 3, 2023

@pddg I got notified and found that the CI was failing because of CircleCI.
So, I disabled the building on CircleCI and removed the required check for the master branch.
Your next push should result in a successful CI.

@pddg pddg requested a review from nojima March 3, 2023 08:34
@pddg pddg self-assigned this Mar 3, 2023
socks/metrics.go Outdated
socksResponseCounter = promauto.With(metrics.Registry).NewCounterVec(prometheus.CounterOpts{
Namespace: metrics.Namespace,
Subsystem: "socks",
Name: "responses_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

このメトリクスの値はリクエスト数と一致するはずなので、requests_total にしたほうが慣習的な気がします

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number of requests and responses do not always seem to match.
This is because if the request is not a correct SOCKS request, these are dropped.

usocksd/socks/server.go

Lines 128 to 156 in 03fb5bd

var preamble [2]byte
_, err := io.ReadFull(conn, preamble[:])
if err != nil {
fields := well.FieldsFromContext(ctx)
fields["client_addr"] = conn.RemoteAddr().String()
fields[log.FnError] = err.Error()
_ = s.Logger.Error("failed to read preamble", fields)
return
}
connVer := version(preamble[0])
var destConn net.Conn
switch connVer {
case SOCKS4:
destConn = s.handleSOCKS4(ctx, conn, preamble[1])
if destConn == nil {
return
}
case SOCKS5:
destConn = s.handleSOCKS5(ctx, conn, preamble[1])
if destConn == nil {
return
}
default:
fields := well.FieldsFromContext(ctx)
fields["client_addr"] = conn.RemoteAddr().String()
_ = s.Logger.Error("unknown SOCKS version", fields)
return
}

Do you think we should count the number of these incorrect requests?

Copy link
Contributor

Choose a reason for hiding this comment

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

I recognized that the word "request" is ambiguous.
How about keeping the number of connections as a metric to clarify what counts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean that even invalid SOCKS protocol connections count?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@pddg
Copy link
Contributor Author

pddg commented Mar 15, 2023

The labels "reason" and "msg" have been removed. These could receive arbitrary error messages and could unnecessarily increase cardinality or contain strings that were not appropriate as labels.

nojima
nojima previously approved these changes Mar 22, 2023
config_test.go Outdated
@@ -30,6 +30,9 @@ func TestConfig(t *testing.T) {
if c.Incoming.Port != 1080 {
t.Error("incoming.port != 1080")
}
if c.Incoming.MetricsPort != 8081 {
t.Error("incoming.port != 1080")
Copy link
Member

Choose a reason for hiding this comment

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

8081?
These days, I use https://github.com/stretchr/testify to write this kind of tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote it by copying and pasting, sorry :(
This is a simple struct comparison, and I believe google/go-cmp is more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix this, soon.

@nojima nojima merged commit da05d21 into master Mar 23, 2023
@nojima nojima deleted the metrics2 branch March 23, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics
4 participants