Skip to content

Conversation

@qianlongzt
Copy link

@qianlongzt qianlongzt commented Oct 22, 2025

Summary

  1. fix Run Domain Expiration checks on the root domain #1159
image image
  1. Refactor Type for simplification and better performance
  2. add missing endpoint type test for grpc feat(client): Add support for monitoring gRPC endpoints #1376

test result for perfomance

cpu: AMD Ryzen 7 4800H with Radeon Graphics
BenchmarkEndpoint_Type-16

Cut + switch 5053628 238.7 ns/op 0 B/op 0 allocs/op
Cut + map 2714998 438.6 ns/op 0 B/op 0 allocs/op
switch HasPerfix 2683519 430.9 ns/op 0 B/op 0 allocs/op

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable.

@github-actions github-actions bot added the feature New feature or request label Oct 22, 2025
@qianlongzt
Copy link
Author

@TwiN Is there anything I can do to help merge PR? Is there no progress?

test result

cpu: AMD Ryzen 7 4800H with Radeon Graphics
BenchmarkEndpoint_Type-16

Cut + switch     5053628 238.7 ns/op 0 B/op 0 allocs/op
Cut + map        2714998 438.6 ns/op 0 B/op 0 allocs/op
switch HasPerfix 2683519 430.9 ns/op 0 B/op 0 allocs/op
@qianlongzt
Copy link
Author

@PythonGermany Your PR qianlongzt#1 looks good to me, but what should I do next?

  1. Merge your PR first and then wait for @TwiN to review?
  2. Or should I not merge it, and instead have you submit your PR to @TwiN after my PR is merged?

@PythonGermany
Copy link
Contributor

@qianlongzt Just merge my changes into your branch.

Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Small nitpicks

Comment on lines +169 to +180
before, _, ok := strings.Cut(e.URL, ":")
if !ok {
return TypeUNKNOWN
}
switch before {
case "tcp":
return TypeTCP
case strings.HasPrefix(e.URL, "sctp://"):
case "sctp":
return TypeSCTP
case strings.HasPrefix(e.URL, "udp://"):
case "udp":
return TypeUDP
case strings.HasPrefix(e.URL, "icmp://"):
case "icmp":
Copy link
Owner

Choose a reason for hiding this comment

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

I understand and appreciate your intention to simplify the code, but personally, having the ability to do a global search for <protocol/endpoint-type>:// (e.g. icmp://) makes navigating the code much easier (reasoning here being that if you search for just http instead of http://, you're going to get hundreds of results as opposed to exactly what you're looking for).

Could you revert the changes here and update the new domain:// prefix accordingly?

Copy link
Author

Choose a reason for hiding this comment

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

Should we add quotation marks to the search string, for example "icmp" and "http"?

Image Image Image

Comment on lines +304 to +310
type testEndpoint_typeArgs struct {
URL string
DNS *dns.Config
SSH *ssh.Config
}

var testEndpoint_typeData = []struct {
Copy link
Owner

Choose a reason for hiding this comment

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

The whole Test<something>_<something> notation is more intended to be for the struct name followed by the method name (and occasionally further details).

Is there any reason why you moved those outside of the function? They're only used in the function, you could just name them typeArgs and typeData within the TestEndpoint_Type test function itself

Copy link
Author

@qianlongzt qianlongzt Jan 3, 2026

Choose a reason for hiding this comment

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

I just want BenchmarkEndpoint_Type to reuse the same test data as TestEndpoint_Type.

This benchmark is for both the old and new Type implementations, and I would like to keep it in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run Domain Expiration checks on the root domain

3 participants