initial version #1
Conversation
WalkthroughAdds a Go client library for the GlobalPing API: introduces a Client interface and NewClient constructor, APIURL constant, and Config type. Implements measurement lifecycle methods (CreateMeasurement, GetMeasurement, AwaitMeasurement, GetMeasurementRaw), Probes and Limits endpoints, and an in-memory ETag-aware cache with CacheClean and CachePurge. Adds comprehensive models and error types for measurements, probes, and limits, unit tests for limits/probes/measurements, go.mod, a GitHub Actions test workflow, README text change, and .gitignore update. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (12)
go.mod (2)
3-3: Use a minor-only Go version in go.mod.Prefer "go 1.24" over "go 1.24.0" to avoid churn in toolchains and accidental diffs.
-go 1.24.0 +go 1.24
5-9: Extract generic utilities from CLI dependency to reduce coupling.The three utility functions used (
TextFromSentence,Pluralize,FormatSeconds) are small, generic string formatting helpers with no CLI-specific logic. They're currently used only in measurements.go for error message formatting. Moving them tointernal/utilseliminates unnecessary library coupling to a CLI module and reduces transitive dependencies for consumers.limits.go (2)
55-66: Graceful fallback when token retrieval fails.A failed refresh (e.g., invalid_grant) prevents fetching public limits; consider proceeding unauthenticated instead of erroring.
- token, err := c.getToken(ctx) - if err != nil { - return nil, &LimitsError{Message: "failed to get token: " + err.Error()} - } + token, err := c.getToken(ctx) + if err != nil { + // If auth is invalid, continue without Authorization to still return public rate limits. + if ae, ok := err.(*AuthorizeError); ok && ae.ErrorType == ErrTypeInvalidGrant { + token = nil + } else { + return nil, &LimitsError{Message: "failed to get token: " + err.Error()} + } + } if token != nil { req.Header.Set("Authorization", token.TokenType+" "+token.AccessToken) }
55-60: Set User-Agent consistently.Other requests set c.userAgent; align here for diagnostics and rate-limiting fairness.
req, err := http.NewRequestWithContext(ctx, "GET", c.apiURL+"/limits", nil) if err != nil { return nil, &LimitsError{Message: "failed to create request - please report this bug"} } + req.Header.Set("User-Agent", c.userAgent)auth.go (2)
174-176: Avoid setting Content-Length manually.http.NewRequest/Client compute Content-Length. Manually setting it risks inconsistencies if bodies change.
- req.Header.Set("Content-Length", strconv.Itoa(len(q.Encode())))Also applies to: 312-314, 375-377, 416-418
437-443: Avoid panicking in generateVerifier.Panics in libraries are hostile to consumers. Return an error up the call chain instead of panicking. If changing signature is too invasive now, at least handle the error in Authorize and abort gracefully.
-func generateVerifier() string { +func generateVerifier() string { data := make([]byte, 32) if _, err := rand.Read(data); err != nil { - panic(err) + // Fallback: time-based pseudo-random; still better than crashing. + return base64.RawURLEncoding.EncodeToString([]byte(strconv.FormatInt(time.Now().UnixNano(), 10))) } return base64.RawURLEncoding.EncodeToString(data) }measurements.go (2)
420-446: Respect context cancellation in polling.AwaitMeasurement sleeps unconditionally; use a context-aware wait to stop immediately on cancellation.
- for m.Status == StatusInProgress { - time.Sleep(500 * time.Millisecond) + for m.Status == StatusInProgress { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(500 * time.Millisecond): + } respBytes, err := c.GetMeasurementRaw(ctx, id) if err != nil { return nil, err } err = json.Unmarshal(respBytes, m) if err != nil { return nil, &MeasurementError{ Message: fmt.Sprintf("invalid get measurement format returned: %v %s", err, string(respBytes)), } } }
142-154: Comment grouping is misleading."DNS" section currently documents HTTP status fields (StatusCode, StatusCodeName). Update comments to reflect actual protocol groups to avoid confusion for users.
README.md (1)
21-38: Resolve markdownlint MD010 (no hard tabs).Replace hard tabs with spaces in code blocks to satisfy MD010.
Also applies to: 40-52
auth_test.go (1)
15-25: Fix typo: succesCalled → successCalled.Improves clarity and avoids future confusion.
- succesCalled := false + successCalled := false ... - if r.URL.Path == "/authorize/success" { - succesCalled = true + if r.URL.Path == "/authorize/success" { + successCalled = true return } ... - assert.True(t, succesCalled, "/authorize/success not called") + assert.True(t, successCalled, "/authorize/success not called")Also applies to: 86-88
measurements_test.go (1)
993-997: Fix misleading comment.The comment says “first request for id1” but the code requests id2.
- // first request for id1 + // first request for id2client.go (1)
102-102: Remove unnecessary explicit zero value initialization.The explicit
sync.RWMutex{}initialization is redundant since Go automatically initializes struct fields to their zero values, and the zero value ofsync.RWMutexis a valid, unlocked mutex.Apply this diff:
c := &client{ - mu: sync.RWMutex{}, authClientId: config.AuthClientID,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
README.md(1 hunks)auth.go(1 hunks)auth_test.go(1 hunks)cache.go(1 hunks)client.go(1 hunks)go.mod(1 hunks)limits.go(1 hunks)limits_test.go(1 hunks)measurements.go(1 hunks)measurements_test.go(1 hunks)utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
limits_test.go (3)
limits.go (6)
LimitsResponse(10-13)RateLimits(15-17)MeasurementsLimits(19-21)MeasurementsCreateLimits(30-35)CreateLimitTypeUser(27-27)CreditLimits(37-39)client.go (2)
NewClient(100-145)Config(65-79)auth.go (1)
Token(28-34)
client.go (3)
measurements.go (3)
MeasurementCreate(82-89)MeasurementCreateResponse(106-109)Measurement(273-282)auth.go (3)
AuthorizeResponse(46-49)IntrospectionResponse(348-364)Token(28-34)limits.go (1)
LimitsResponse(10-13)
auth_test.go (2)
client.go (2)
NewClient(100-145)Config(65-79)auth.go (4)
Token(28-34)IntrospectionResponse(348-364)AuthorizeError(36-40)ErrTypeNotAuthorized(25-25)
measurements_test.go (3)
client.go (2)
NewClient(100-145)Config(65-79)measurements.go (13)
MeasurementCreate(82-89)MeasurementError(91-96)StatusUnauthorizedWithTokenRefreshed(27-27)MeasurementTypePing(75-75)StatusFinished(128-128)DecodePingStats(520-527)MeasurementTypeTraceroute(76-76)MeasurementTypeDNS(77-77)DecodeDNSTimings(556-563)MeasurementTypeMTR(78-78)MeasurementTypeHTTP(79-79)DecodeHTTPTimings(583-590)Measurement(273-282)auth.go (1)
Token(28-34)
🪛 markdownlint-cli2 (0.18.1)
README.md
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
31-31: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
24-25: Consider adding-raceflag for concurrent safety testing.A client library should validate thread safety. The
-raceflag detects data races, which is especially important for code that may be used concurrently.Apply this diff:
- name: Run tests - run: go test ./... -v -timeout 30s + run: go test ./... -v -race -timeout 30s
15-16: Consider testing against multiple Go versions.For initial release, testing only Go 1.23 is narrow. Most libraries test against a range (e.g., 1.22–1.23) to catch compatibility regressions.
Update the matrix to test multiple versions:
matrix: - go: ["1.23"] + go: ["1.22", "1.23"] os: [ubuntu-latest]Check go.mod to confirm the minimum Go version and align the matrix accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (1)
.github/workflows/test.yml (1)
24-25: Verify the 30s timeout is adequate for the full test suite.Given the PR includes comprehensive unit tests across auth, measurements, limits, and caching modules, confirm that 30 seconds is sufficient. If tests approach or exceed this timeout, increase it to avoid flaky CI behavior.
Run the test suite locally and check if execution time is well under 30s with headroom.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
README.md (1)
25-29: Avoid invalid Expiry example (use a sane duration).Using time.Now().Add(math.MaxInt64) overflows on some platforms and is misleading for short‑lived tokens. Show a realistic expiry (or omit it).
- AuthToken: &globalping.Token{ - AccessToken: "<your_access_token>", - Expiry: time.Now().Add(math.MaxInt64), - }, + AuthToken: &globalping.Token{ + AccessToken: "<your_access_token>", + // Example: set a reasonable future expiry if your token doesn't include one. + Expiry: time.Now().Add(24 * time.Hour), + },cache.go (3)
15-19: CachePurge writes under RLock — switch to exclusive Lock.Mutating c.cache while holding RLock violates RWMutex semantics and risks races.
func (c *client) CachePurge() { - c.mu.RLock() - defer c.mu.RUnlock() + c.mu.Lock() + defer c.mu.Unlock() c.cache = map[string]*cacheEntry{} }
21-29: getETag should honor expiry and evict stale entries.Currently returns ETags even when expired. Treat expired as miss and remove it safely.
func (c *client) getETag(id string) string { - c.mu.RLock() - defer c.mu.RUnlock() - e, ok := c.cache[id] - if !ok { - return "" - } - return e.ETag + now := time.Now().Unix() + c.mu.RLock() + e, ok := c.cache[id] + if !ok { + c.mu.RUnlock() + return "" + } + if e.ExpireAt > 0 && e.ExpireAt < now { + // upgrade to write lock to delete + c.mu.RUnlock() + c.mu.Lock() + if ee, ok2 := c.cache[id]; ok2 && ee.ExpireAt > 0 && ee.ExpireAt < now { + delete(c.cache, id) + } + c.mu.Unlock() + return "" + } + et := e.ETag + c.mu.RUnlock() + return et }
31-39: getCachedResponse ignores expiry and exposes internal slice.
- Serve nil for expired entries.
- Return a copy to avoid external mutation/data races.
func (c *client) getCachedResponse(id string) []byte { - c.mu.RLock() - defer c.mu.RUnlock() - e, ok := c.cache[id] - if !ok { - return nil - } - return e.Data + now := time.Now().Unix() + c.mu.RLock() + e, ok := c.cache[id] + if !ok { + c.mu.RUnlock() + return nil + } + if e.ExpireAt > 0 && e.ExpireAt < now { + c.mu.RUnlock() + return nil + } + // copy to prevent callers from mutating internal cache + b := make([]byte, len(e.Data)) + copy(b, e.Data) + c.mu.RUnlock() + return b }measurements_test.go (1)
956-986: Close test server to avoid leaking ports/descriptors.Add
defer s.Close()right afterhttptest.NewServer(...).- s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ... - })) + })) + defer s.Close()client.go (2)
78-78: Document or set a default for cache expiration.
CacheExpireSecondsdefaults to 0 when not set, which likely disables caching or causes immediate expiration. Either document this behavior clearly or apply a sensible default (e.g., 300 seconds) inNewClient.
86-87: Fix inconsistent field naming.Field names should follow Go conventions where abbreviations like
IDare fully capitalized. ChangeauthClientIdtoauthClientIDon line 86 to match theConfigstruct and Go naming standards. Update all references throughout the file (e.g., lines 96, 103, 161 in auth.go).auth.go (6)
76-106: Bind the local callback server to loopback and add CSRF state.The OAuth flow has two security concerns:
- Line 80:
net.Listen("tcp", ":"+port)binds to all interfaces. Bind to"127.0.0.1:"+portinstead.- Lines 95-101: Missing CSRF protection. Generate a random state parameter, include it in the authorize URL, and validate it in the callback handler.
167-207: Close HTTP response body to avoid connection leak.After
c.http.Do(req)on line 176, adddefer resp.Body.Close()to ensure the connection is released in all code paths.Apply this diff:
resp, err := c.http.Do(req) if err != nil { return nil, &AuthorizeError{ ErrorType: ErrTypeExchangeFailed, Description: err.Error(), } } + defer resp.Body.Close()
209-243: Data race: writing under RLock in getToken.
getTokenholds a read lock (RLock) but then mutatesc.tokenon line 232 and calls theonTokenRefreshcallback. This violates Go's RWMutex contract and can cause data races.Release the read lock before calling
refreshToken, then acquire a write lock to update state.
299-345: Close HTTP response body to avoid connection leak.After
c.http.Do(req)on line 314, adddefer resp.Body.Close().Apply this diff:
resp, err := c.http.Do(req) if err != nil { return nil, &AuthorizeError{ ErrorType: ErrTypeRefreshFailed, Description: err.Error(), } } + defer resp.Body.Close()
366-402: Close HTTP response body to avoid connection leak.After
c.http.Do(req)on line 377, adddefer resp.Body.Close().Apply this diff:
resp, err := c.http.Do(req) if err != nil { return nil, &AuthorizeError{ ErrorType: ErrTypeIntrospectionFailed, Description: err.Error(), } } + defer resp.Body.Close()
404-435: Close HTTP response body to avoid connection leak.After
c.http.Do(req)on line 418, adddefer resp.Body.Close().Apply this diff:
resp, err := c.http.Do(req) if err != nil { return &AuthorizeError{ ErrorType: ErrTypeRevokeFailed, Description: err.Error(), } } + defer resp.Body.Close()
🧹 Nitpick comments (7)
README.md (1)
21-55: Replace hard tabs in code blocks to satisfy markdownlint MD010 (no-hard-tabs).The sample uses tabs for indentation; markdownlint flags lines 21–54. Replace tabs with spaces or disable MD010 for code blocks.
utils_test.go (1)
3-10: Use "Bearer" capitalization to mirror real OAuth responses.Some servers and middleware expect the scheme-cased "Bearer". Align the fixture for better fidelity.
- "token_type":"bearer", + "token_type":"Bearer",.github/workflows/test.yml (1)
15-16: Drive Go version from go.mod and enable cache to avoid drift and speed up CI.Using a hard-coded matrix can desync from go.mod. Use go-version-file and turn on module cache.
- matrix: - go: ["1.24"] - os: [ubuntu-latest] + matrix: + os: [ubuntu-latest] @@ - - name: Setup go - uses: actions/setup-go@v6 - with: - go-version: ${{ matrix.go }} + - name: Setup go + uses: actions/setup-go@v6 + with: + go-version-file: go.mod + cache: trueAlso applies to: 21-25
go.mod (1)
3-3: Use canonical Go directive without patch level.The go directive typically omits patch (e.g.,
go 1.24). Using1.24.0can break older tooling. Align with CI.-go 1.24.0 +go 1.24If you want to pin a specific toolchain, add a separate
toolchain go1.24.xline.cache.go (1)
41-60: Copy response bytes on insert to avoid aliasing.If callers reuse or mutate resp after caching, the cache content changes unexpectedly.
func (c *client) cacheResponse(id string, etag string, resp []byte) { c.mu.Lock() defer c.mu.Unlock() + // store a copy to decouple from caller's buffer + data := append([]byte(nil), resp...) @@ - e.Data = resp + e.Data = data @@ - Data: resp, + Data: data,limits.go (1)
55-67: Set standard headers (User-Agent, Accept) for consistency.Some APIs use UA for diagnostics and require Accept: application/json.
req, err := http.NewRequestWithContext(ctx, "GET", c.apiURL+"/limits", nil) @@ if token != nil { req.Header.Set("Authorization", token.TokenType+" "+token.AccessToken) } +if c.userAgent != "" { + req.Header.Set("User-Agent", c.userAgent) +} +req.Header.Set("Accept", "application/json")measurements.go (1)
419-445: Consider adding context cancellation check in polling loop.The polling loop on line 431 doesn't check for context cancellation, so it may continue polling even after the context is cancelled. Consider adding a select statement with
ctx.Done()to allow early termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)auth.go(1 hunks)auth_test.go(1 hunks)cache.go(1 hunks)client.go(1 hunks)go.mod(1 hunks)limits.go(1 hunks)limits_test.go(1 hunks)measurements.go(1 hunks)measurements_test.go(1 hunks)utils.go(1 hunks)utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- limits_test.go
- auth_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
measurements_test.go (3)
client.go (2)
NewClient(100-145)Config(65-79)measurements.go (13)
MeasurementCreate(81-88)MeasurementError(90-95)StatusUnauthorizedWithTokenRefreshed(26-26)MeasurementTypePing(74-74)StatusFinished(127-127)DecodePingStats(519-526)MeasurementTypeTraceroute(75-75)MeasurementTypeDNS(76-76)DecodeDNSTimings(555-562)MeasurementTypeMTR(77-77)MeasurementTypeHTTP(78-78)DecodeHTTPTimings(582-589)Measurement(272-281)auth.go (1)
Token(28-34)
client.go (3)
measurements.go (3)
MeasurementCreate(81-88)MeasurementCreateResponse(105-108)Measurement(272-281)auth.go (3)
AuthorizeResponse(46-49)IntrospectionResponse(348-364)Token(28-34)limits.go (1)
LimitsResponse(10-13)
measurements.go (1)
utils.go (3)
TextFromSentence(48-50)Pluralize(22-27)FormatSeconds(9-20)
🪛 markdownlint-cli2 (0.18.1)
README.md
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
27-27: Hard tabs
Column: 1
(MD010, no-hard-tabs)
28-28: Hard tabs
Column: 1
(MD010, no-hard-tabs)
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
31-31: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
auth.go (1)
79-99: Bind callback server to loopback and add/validate OAuth state.Listening on “:port” exposes the local callback externally, and missing state enables CSRF on the redirect. Bind to 127.0.0.1, use 127.0.0.1 in the redirect, include a random state, and validate it in the handler.
@@ - for i := range ports { + for i := range ports { port = strconv.Itoa(ports[i]) - ln, err = net.Listen("tcp", ":"+port) + ln, err = net.Listen("tcp", "127.0.0.1:"+port) if err == nil { break } } @@ - callbackURL = "http://localhost:" + port + "/callback" + callbackURL = "http://127.0.0.1:" + port + "/callback" + state, err := generateVerifier() + if err != nil { return nil, err } @@ q := url.Values{} q.Set("client_id", c.authClientID) q.Set("code_challenge", generateS256Challenge(verifier)) q.Set("code_challenge_method", "S256") q.Set("response_type", "code") q.Set("scope", "measurements") q.Set("redirect_uri", callbackURL) + q.Set("state", state) @@ - mux.HandleFunc("/callback", func(w http.ResponseWriter, req *http.Request) { + mux.HandleFunc("/callback", func(w http.ResponseWriter, req *http.Request) { req.ParseForm() + if req.Form.Get("state") != state { + http.Redirect(w, req, c.dashboardURL+"/authorize/error", http.StatusFound) + go func() { + server.Shutdown(context.Background()) + callback(&AuthorizeError{ErrorType: "state_mismatch", Description: "invalid state"}) + }() + return + } token, err := c.exchange(ctx, req.Form, verifier, callbackURL)Also applies to: 62-77, 97-105
🧹 Nitpick comments (7)
.gitignore (1)
1-1: Consider adding common Go project patterns to.gitignore.The
bin/entry is a solid start, but a typical Go project.gitignoreincludes patterns for IDE files, OS artifacts, and test coverage output.bin/ +.vscode/ +.idea/ +*.swp +*.swo +.DS_Store +coverage.out +*.testlimits.go (2)
14-20: Don’t fail on token retrieval; call /limits unauthenticated when auth isn’t available.Proceed without Authorization if getToken errors, instead of returning early. This endpoint returns useful data without auth; degrade gracefully.
Apply:
- token, err := c.getToken(ctx) - if err != nil { - return nil, &LimitsError{Message: "failed to get token: " + err.Error()} - } - if token != nil { - req.Header.Set("Authorization", token.TokenType+" "+token.AccessToken) - } + token, err := c.getToken(ctx) + if err == nil && token != nil { + req.Header.Set("Authorization", token.TokenType+" "+token.AccessToken) + }
9-13: Align headers and content-decoding with the rest of the client.Set User-Agent and support Brotli like other endpoints to avoid inconsistent behavior if the server compresses responses.
Example:
import ( "context" "encoding/json" + "io" "net/http" + "github.com/andybalholm/brotli" ) func (c *client) Limits(ctx context.Context) (*LimitsResponse, error) { req, err := http.NewRequestWithContext(ctx, "GET", c.apiURL+"/limits", nil) if err != nil { return nil, &LimitsError{Message: "failed to create request - please report this bug"} } + req.Header.Set("User-Agent", c.userAgent) + req.Header.Set("Accept-Encoding", "br") @@ resp, err := c.http.Do(req) @@ - limits := &LimitsResponse{} - err = json.NewDecoder(resp.Body).Decode(limits) + var body io.Reader = resp.Body + if resp.Header.Get("Content-Encoding") == "br" { + body = brotli.NewReader(body) + } + limits := &LimitsResponse{} + err = json.NewDecoder(body).Decode(limits)Also applies to: 21-25, 37-41
measurement.go (2)
165-191: Respect context cancellation in AwaitMeasurement to avoid indefinite waits.Break the polling loop when ctx is done and propagate the cancellation.
func (c *client) AwaitMeasurement(ctx context.Context, id string) (*Measurement, error) { @@ - for m.Status == StatusInProgress { - time.Sleep(500 * time.Millisecond) + for m.Status == StatusInProgress { + select { + case <-ctx.Done(): + return nil, &MeasurementError{Message: ctx.Err().Error()} + case <-time.After(500 * time.Millisecond): + } respBytes, err := c.GetMeasurementRaw(ctx, id)
66-76: Deterministic validation error text (map iteration order).Building the message by ranging over a map yields non-deterministic ordering when multiple params exist. Sort keys for stable, testable output.
- resErr := "" - for _, v := range data.Error.Params { - resErr += fmt.Sprintf(" - %s\n", v) - } + keys := make([]string, 0, len(data.Error.Params)) + for k := range data.Error.Params { + keys = append(keys, k) + } + sort.Strings(keys) + var b strings.Builder + for _, k := range keys { + fmt.Fprintf(&b, " - %v\n", data.Error.Params[k]) + } @@ - err.Message = fmt.Sprintf("invalid parameters:\n%s", resErr) + err.Message = fmt.Sprintf("invalid parameters:\n%s", strings.TrimRight(b.String(), "\n"))Remember to import sort and strings.
client.go (1)
81-81: Clarify the meaning of zero value in the field documentation.The
CacheExpireSecondsfield comment doesn't explain what0means, but line 102 clarifies it means "no expiration". Document this behavior directly on the field for better discoverability.Apply this diff:
- CacheExpireSeconds int64 // Cache entry expiration time in seconds + CacheExpireSeconds int64 // Cache entry expiration time in seconds. 0 means no expiration.measurement_models.go (1)
33-37: Consider making protocol slices immutable.These exported variables can be modified by client code, potentially causing unexpected behavior. If these represent fixed protocol sets, consider documenting that they should not be modified, or providing them through a function that returns a copy.
Example approach - return copies via functions:
func PingProtocols() []string { return []string{"ICMP", "TCP"} } func TracerouteProtocols() []string { return []string{"ICMP", "TCP", "UDP"} } func DNSProtocols() []string { return []string{"TCP", "UDP"} } func MTRProtocols() []string { return []string{"ICMP", "TCP", "UDP"} } func HTTPProtocols() []string { return []string{"HTTP", "HTTPS", "HTTP2"} }Alternatively, if keeping as variables, document the expectation:
+// PingProtocols defines the supported protocols for ping measurements. +// This slice should not be modified. var PingProtocols = []string{"ICMP", "TCP"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)auth.go(1 hunks)cache.go(1 hunks)client.go(1 hunks)limits.go(1 hunks)limits_models.go(1 hunks)measurement.go(1 hunks)measurement_models.go(1 hunks)measurement_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cache.go
🧰 Additional context used
🧬 Code graph analysis (4)
measurement.go (2)
measurement_models.go (14)
MeasurementCreate(60-67)MeasurementCreateResponse(84-87)MeasurementError(69-74)MeasurementErrorResponse(80-82)Measurement(251-260)StatusInProgress(103-103)PingTiming(149-152)PingStats(138-147)TracerouteHop(158-162)DNSAnswer(164-170)TraceDNSHop(176-180)DNSTimings(172-174)MTRHop(202-208)HTTPTimings(210-217)utils.go (3)
TextFromSentence(48-50)Pluralize(22-27)FormatSeconds(9-20)
measurement_test.go (4)
client.go (2)
NewClient(103-148)Config(68-82)measurement_models.go (9)
MeasurementCreate(60-67)MeasurementError(69-74)MeasurementTypePing(53-53)StatusFinished(106-106)MeasurementTypeTraceroute(54-54)MeasurementTypeDNS(55-55)MeasurementTypeMTR(56-56)MeasurementTypeHTTP(57-57)Measurement(251-260)auth.go (1)
Token(28-34)measurement.go (4)
StatusUnauthorizedWithTokenRefreshed(26-26)DecodePingStats(265-272)DecodeDNSTimings(301-308)DecodeHTTPTimings(328-335)
client.go (3)
measurement_models.go (3)
MeasurementCreate(60-67)MeasurementCreateResponse(84-87)Measurement(251-260)auth.go (3)
AuthorizeResponse(46-49)IntrospectionResponse(359-375)Token(28-34)limits_models.go (1)
LimitsResponse(5-8)
limits.go (1)
limits_models.go (3)
LimitsResponse(5-8)LimitsError(36-40)LimitsErrorResponse(46-48)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
client.go (1)
102-102: Documentation still references wrong method name.The comment mentions
CleanCache, but the actual method isCacheClean()(line 62).measurement_models.go (1)
43-43: Port field still missingomitemptytag.As flagged in previous reviews, the
Portfield is the only field withoutomitempty, creating an inconsistency. Sinceuint16defaults to0(not a valid port), it should not serialize when unset.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client.go(1 hunks)measurement_models.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client.go (3)
measurement_models.go (3)
MeasurementCreate(60-67)MeasurementCreateResponse(84-87)Measurement(251-260)auth.go (3)
AuthorizeResponse(46-49)IntrospectionResponse(359-375)Token(28-34)limits_models.go (1)
LimitsResponse(5-8)
🔇 Additional comments (2)
client.go (1)
103-148: LGTM with minor note.The constructor correctly applies defaults, initializes the HTTP client with a sensible timeout, and defensively copies the auth token. The default "Bearer" token type (line 144) is a reasonable fallback.
measurement_models.go (1)
109-260: Well-structured measurement models.The use of
json.RawMessagefor polymorphic fields (lines 118, 124, 130, 134-135) is a solid approach that allows type-specific decoding later. The comprehensive coverage of different measurement types (Ping, DNS, HTTP, MTR, Traceroute) with their respective result structures demonstrates thoughtful API modeling.
MartinKolarik
left a comment
MartinKolarik
left a comment
There was a problem hiding this comment.
This is meant to be a general-purpose wrapper of our API that any app can build upon. That means the methods should be simple wrappers of the API endpoints with just small things on top (types, compression, etag, and generally following our client guidelines).
Things like interacting with the OAuth server and refreshing tokens are out of scope since each app may want to handle them differently.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
limits_test.go (1)
33-33: Handle json.Marshal error in test server.Ignoring the error can cause the test to pass invalid payloads silently.
- b, _ := json.Marshal(expectedResponse) + b, err := json.Marshal(expectedResponse) + if err != nil { + t.Fatal(err) + }client.go (1)
55-55: Document the behavior whenCacheExpireSecondsis 0.It's unclear whether
0means "no expiration" or "caching disabled." Document this in the field comment to avoid confusion.Apply this diff:
- CacheExpireSeconds int64 // Cache entry expiration time in seconds. 0 means no expiration. + CacheExpireSeconds int64 // Cache entry expiration time in seconds. 0 means cache entries never expire.measurement_models.go (1)
35-39: Protocol slices remain mutable and can be corrupted by external code.These exported slice variables can be modified by callers (e.g.,
PingProtocols[0] = "INVALID"), potentially breaking validation logic.Consider making them unexported with getter functions that return copies:
-var PingProtocols = []string{"ICMP", "TCP"} -var TracerouteProtocols = []string{"ICMP", "TCP", "UDP"} -var DNSProtocols = []string{"TCP", "UDP"} -var MTRProtocols = []string{"ICMP", "TCP", "UDP"} -var HTTPProtocols = []string{"HTTP", "HTTPS", "HTTP2"} +var ( + pingProtocols = []string{"ICMP", "TCP"} + tracerouteProtocols = []string{"ICMP", "TCP", "UDP"} + dnsProtocols = []string{"TCP", "UDP"} + mtrProtocols = []string{"ICMP", "TCP", "UDP"} + httpProtocols = []string{"HTTP", "HTTPS", "HTTP2"} +) + +func GetPingProtocols() []string { return append([]string{}, pingProtocols...) } +func GetTracerouteProtocols() []string { return append([]string{}, tracerouteProtocols...) } +func GetDNSProtocols() []string { return append([]string{}, dnsProtocols...) } +func GetMTRProtocols() []string { return append([]string{}, mtrProtocols...) } +func GetHTTPProtocols() []string { return append([]string{}, httpProtocols...) }
🧹 Nitpick comments (2)
limits.go (1)
10-58: Consider extracting common HTTP GET pattern.The Limits and Probes implementations follow an identical pattern. A generic helper could reduce duplication:
func (c *client) doGet(ctx context.Context, path string, result interface{}, errResult interface{}) errorThis refactor can be deferred to a future PR.
client.go (1)
10-12: Preferconstfor immutable values.
APIURLis declared as avarbut holds a constant string that shouldn't change at runtime.Apply this diff:
-var ( - APIURL = "https://api.globalping.io/v1" -) +const APIURL = "https://api.globalping.io/v1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)client.go(1 hunks)limits.go(1 hunks)limits_models.go(1 hunks)limits_test.go(1 hunks)measurement.go(1 hunks)measurement_models.go(1 hunks)measurement_test.go(1 hunks)probes.go(1 hunks)probes_models.go(1 hunks)probes_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
probes_test.go (2)
probes_models.go (2)
ProbesResponse(32-32)ProbeLocation(11-21)client.go (3)
APIURL(11-11)NewClient(74-96)Config(52-58)
limits.go (2)
limits_models.go (3)
LimitsResponse(10-13)LimitsErrorResponse(52-54)LimitsError(41-46)client.go (1)
APIURL(11-11)
probes.go (2)
probes_models.go (3)
ProbesResponse(32-32)ProbesErrorResponse(47-49)ProbesError(35-40)client.go (1)
APIURL(11-11)
measurement_test.go (3)
client.go (3)
NewClient(74-96)Config(52-58)APIURL(11-11)measurement_models.go (10)
MeasurementCreate(62-69)MeasurementCreateResponse(93-96)MeasurementError(71-77)MeasurementTypePing(55-55)StatusFinished(115-115)MeasurementTypeTraceroute(56-56)MeasurementTypeDNS(57-57)MeasurementTypeMTR(58-58)MeasurementTypeHTTP(59-59)Measurement(260-269)measurement.go (3)
DecodePingStats(195-202)DecodeDNSTimings(231-238)DecodeHTTPTimings(258-265)
measurement_models.go (1)
probes_models.go (1)
Probe(24-29)
client.go (3)
measurement_models.go (3)
MeasurementCreate(62-69)MeasurementCreateResponse(93-96)Measurement(260-269)limits_models.go (1)
LimitsResponse(10-13)probes_models.go (1)
ProbesResponse(32-32)
measurement.go (2)
measurement_models.go (14)
MeasurementCreate(62-69)MeasurementCreateResponse(93-96)MeasurementErrorResponse(89-91)MeasurementError(71-77)Measurement(260-269)StatusInProgress(112-112)PingTiming(158-161)PingStats(147-156)TracerouteHop(167-171)DNSAnswer(173-179)TraceDNSHop(185-189)DNSTimings(181-183)MTRHop(211-217)HTTPTimings(219-226)client.go (1)
APIURL(11-11)
limits_test.go (2)
limits_models.go (6)
LimitsResponse(10-13)RateLimits(15-17)MeasurementsLimits(19-21)MeasurementsCreateLimits(30-35)CreateLimitTypeUser(27-27)CreditLimits(37-39)client.go (3)
APIURL(11-11)NewClient(74-96)Config(52-58)
🪛 markdownlint-cli2 (0.18.1)
README.md
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
123-123: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Bare URL used
(MD034, no-bare-urls)
🔍 Remote MCP
Let me try web search to gather relevant information about the GlobalPing API and Go client best practices:## PR Review: Globalping Go API Client - Initial Version
Based on my research using available sources, here is the relevant context for reviewing this PR:
GlobalPing API Context
The Globalping API provides access to thousands of global probes to build tools and integrations. Users can perform ping, traceroute, and DNS lookups from a globally distributed network of user-hosted probes, along with MTR and HTTP measurements. The API supports both authenticated (with higher rate limits) and unauthenticated access.
Client Implementation Assessment
Cache Implementation with Thread Safety:
The PR introduces an in-memory cache with thread-safe operations using mutex locking. Mutexes in Go are used wherever shared resources need to be accessed by multiple goroutines concurrently, allowing only one goroutine at a time to access a critical section of code. This pattern is appropriate for the cache subsystem managing ETags and response data.
ETag-Based Caching:
The implementation supports ETag-based conditional requests using the If-None-Match header. A transport implementation adds validators (etag/if-modified-since) to repeated requests, allowing servers to return 304 Not Modified responses. If there is a stale response, validators will be set on new requests, and if a 304 is received, the cached response will be returned. This is a correct HTTP caching pattern for the client.
Brotli Compression Support:
The PR includes Brotli compression support (via the github.com/andybalholm/brotli dependency in go.mod). This aligns with modern API practices and reduces bandwidth consumption for measurement payloads.
API Surface Design
The client provides a well-structured interface matching the GlobalPing API endpoints:
- Measurement lifecycle methods (CreateMeasurement, GetMeasurement, AwaitMeasurement, GetMeasurementRaw)
- Probes endpoint for listing available probes
- Limits endpoint for checking rate limits and credits
- Cache management methods (CacheClean, CachePurge)
Testing Coverage:
The PR includes comprehensive test coverage for:
- All four measurement types (ping, traceroute, DNS, MTR, HTTP)
- Authentication flows and token handling
- Error scenarios across HTTP status codes
- Cache behavior with ETags and Brotli compression
CI/CD Pipeline:
The GitHub Actions workflow (test.yml) properly configures testing with Go 1.24, the race detector enabled, and a 30-second timeout, which helps catch concurrent access issues during CI runs.
Key Observations for Review
- Concurrency Safety: The cache uses mutexes correctly to protect shared state across goroutines, following Go best practices
- HTTP Caching: ETag implementation follows standard HTTP conditional request semantics
- Comprehensive Model Coverage: Supports all major GlobalPing measurement types with detailed result structures
- Error Handling: Implements proper error wrapping for API responses with nested error details
- Configuration: The Config struct allows custom HTTP clients, user agents, and authentication tokens
[::DeepWiki::]] [::web_search::]]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
measurement.go (1)
103-115: Check context cancellation in the polling loop.The loop polls every 500ms but never checks if
ctxis cancelled, preventing users from aborting long-running measurements. This was flagged in a previous review and remains unresolved.Apply this diff:
for m.Status == StatusInProgress { - time.Sleep(500 * time.Millisecond) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(500 * time.Millisecond): + } respBytes, err := c.GetMeasurementRaw(ctx, id)
🧹 Nitpick comments (2)
README.md (1)
20-24: Declarectxin the example.The examples throughout the README use
ctxbut never show where it comes from. Add a line declaring the context to make the example complete and runnable.Apply this diff:
func main() { + ctx := context.Background() client := globalping.NewClient(globalping.Config{ AuthToken: "<your_access_token>", // Optional })measurement.go (1)
189-268: Consider extracting common decode pattern.All decoder functions follow an identical pattern: unmarshal into a typed variable, return a generic error message on failure. While not critical, extracting this into a generic helper could reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)measurement.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
measurement.go (2)
measurement_models.go (14)
MeasurementCreate(62-69)MeasurementCreateResponse(93-96)MeasurementErrorResponse(89-91)MeasurementError(71-77)Measurement(260-269)StatusInProgress(112-112)PingTiming(158-161)PingStats(147-156)TracerouteHop(167-171)DNSAnswer(173-179)TraceDNSHop(185-189)DNSTimings(181-183)MTRHop(211-217)HTTPTimings(219-226)client.go (1)
APIURL(11-11)
🪛 markdownlint-cli2 (0.18.1)
README.md
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
123-123: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
measurement.go (1)
14-76: LGTM: Unmarshaling error now properly returned.The previous issue about silently dropping the JSON decode error has been fixed. Lines 70-73 now correctly check and return the unmarshaling error.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
138-138: Bare URL used
(MD034, no-bare-urls)
🔍 Remote MCP DeepWiki
Perfect! Now I have enough context. Let me compile the findings:
Summary of Additional Context for PR Review
Based on comprehensive research across multiple sources, here is relevant information for reviewing this GlobalPing Go client pull request:
GlobalPing API Specification
The PR implementation aligns with the official GlobalPing API structure:
- Correct endpoints: The client implements all primary endpoints (
/v1/measurements,/v1/probes,/v1/limits) - Five measurement types are properly supported in the models: ping, traceroute, dns, mtr, and http - matching the API specification
- Measurement lifecycle correctly implements: creation, retrieval, polling (AwaitMeasurement), and raw access with response status progression through
in-progressandfinishedstates - Error response structure properly handles the API's error format with nested
errorobjects containingtypeandmessagefields
Authentication & Rate Limiting
The implementation includes:
- Bearer token authentication correctly implemented via Authorization header injection, matching API's authentication model
- Rate limiting awareness through the
/v1/limitsendpoint implementation for reading rate limit headers (X-RateLimit-*) - Token refresh capability acknowledged through the
OnTokenRefreshpattern in tests, though the full OAuth2 refresh flow would need verification
HTTP Client Best Practices
The client follows Go best practices:
- Client reuse: Properly stores
http.Clientand reuses it across requests (efficiency recommendation met) - Timeout configuration: Defaults to 30-second timeout if no HTTPClient provided
- Context usage: All methods properly accept
context.Contextfor cancellation and deadline management - Response body handling: Should verify all response bodies are properly closed (standard requirement)
Concurrent-Safe Caching
The caching implementation:
- Mutex protection: Uses
sync.Mutexfor thread-safe access to cache entries (correct approach for concurrent reads/writes) - RWMutex consideration: Could potentially optimize using
sync.RWMutexsince caching is read-heavy, but currentMutexapproach is valid - Cache expiration logic: Properly implements expiration-aware cleanup with
ExpireAtfield and cleanup function
ETag-Based Caching
The conditional request implementation:
- If-None-Match header pattern: Correctly matches standard HTTP caching practice using
If-None-Matchheader with ETag values - 304 Not Modified handling: Properly caches responses and returns cached data on 304 responses
- Brotli compression support: Correctly integrates via dependency (
github.com/andybalholm/brotli v1.2.0)
Go 1.24 Compatibility
The PR targets Go 1.24 which introduces:
- Post-quantum cryptography: New X25519MLKEM768 hybrid key exchange enabled by default (client should be compatible)
- FIPS 140-3 support: Via
fips140setting, though not required for this client - Networking improvements: Multipath TCP enhancements (standard http.Client will inherit benefits)
- No breaking language changes: Go 1.24 changes are primarily behavioral/library-level
Key Implementation Observations
- Response format consistency: All endpoints (limits, measurements, probes) implement consistent error wrapping pattern with status codes and error messages
- Measurement models comprehensiveness: All measurement result types (PingStats, TracerouteHop, DNSAnswer, MTRHop, HTTPTimings) are defined with proper TLS certificate metadata
- Test coverage: Includes tests for authentication flows, token refresh, Brotli decompression, ETag caching, and multiple measurement types
2 participants
No description provided.