Repository navigation
fix: align client with API docs and rewrite documentation - #1
Conversation
martende
changed the title
martende
changed the title
WithUserAgent only rejected CR and LF, although its error message promised to reject control characters. A value such as "app\x00" built a client whose every request failed in net/http with an invalid header error, which was then retried as a TransportError. Also remove the unused (*Client).redact helper; the redaction test now exercises the per-call redactor directly.
Restructure the README around common tasks: installation, quick start, fetching data, configuration with a defaults table, error handling and examples. Fix the GOPRIVATE command, which overwrote existing entries, and the backoff description, which implied a one-second minimum delay. Link the API key setup page, use a real CI badge and gofmt-formatted snippets, and describe OAuth support as accepting existing tokens rather than implying a login flow. Turn docs/design.md from an implementation spec into design notes that explain the decisions and what is out of scope; the glassnode-cli migration plan belongs in its tracker. Expand the package doc shown by go doc, document the examples' flags and environment variables, and describe v0.1.0 in the changelog by its features instead of pre-release review history.
Compared the client against docs.glassnode.com and fixed what differed: - Bulk entries carry every selector of their parameter combination (e, network, category, ...), but BulkEntry kept only a and network, so entries for different exchanges or object-metric categories were indistinguishable. BulkEntry.Params now holds all of them and encodes back to the wire format. The API requires s for bulk requests, so GetBulkMetric rejects a request without Since before sending it. - The API reports its per-minute rate limit through x-rate-limit-reset and does not document Retry-After; retrying a 429 after the default jittered delays only hit the limit again. A 429 without Retry-After now uses x-rate-limit-reset as the minimum delay and returns the error immediately when that exceeds MaxDelay. The requested wait is exposed as APIError.RetryAfter. - parameters_defaults values are strings in the documentation and lists in recorded responses; accept both instead of failing the whole metadata call. - Add ListExchanges, ListNetworks and ListMiners for the documented metadata endpoints. Document the bulk time-range limits and credit cost, the rate-limit handling and the new methods.
Requested every metric listed by the API over a short range and compared the responses with what the client expects. - Every point-in-time metric (843 of 1818) adds computed_at to each point, the time at which the value was computed; TimeSeriesPoint and ObjectTimeSeriesPoint silently dropped it. Add ComputedAt as a nullable field and record two real PIT responses as contract fixtures. - GetAPIUsage required apiAddons; an account without add-ons can report null or omit the field. Only creditsUsed is required now. - A non-2xx body was read up to 4 KiB and closed, which discards the connection when the body is longer. Drain a bounded remainder so retries after 429 and 5xx reuse the connection.
…eams The API's rate limit works in one-minute windows and a 429 reports the seconds until the window resets, up to 60. With the default MaxDelay of 30 seconds the client returned a 429 from the first half of a window without retrying at all. Default to 65 seconds; explicit policies are unchanged. Get decoded responses from a fully buffered body, holding both the bytes and the decoded values in memory. Decode from the response stream instead, so memory use is roughly the decoded values. Read failures during decoding are still transport errors and are retried; invalid JSON is still a DecodeError and is not. Public API clients such as go-github, stripe-go and slack-go impose no response size limit, since the server decides how much a request returns. Follow that default, and add WithMaxResponseBytes for callers who prefer a failing call to unbounded memory; such a response fails with ErrResponseTooLarge and is not retried.
Standard linters plus gosec, revive, errorlint, bodyclose and a few others. Fixes: unchecked Close and io.Copy results, a never-used return value of do, an unnecessary conversion, an ineffectual assignment in a test and missing comments on exported UnmarshalJSON methods. Retry jitter keeps math/rand (G404 excluded): it needs no cryptographic randomness.
- Lint both modules with golangci-lint; gofmt and go vet move into it. - Run govulncheck on the latest Go. - Pin actions by commit SHA and let Dependabot update them weekly. - On a vX.Y.Z tag, run the tests and create a GitHub release whose notes are that version's CHANGELOG section; the tag fails without one. - Run CI only for pull requests and pushes to main, cancelling stale runs of the same ref.
Install with a plain go get and link pkg.go.dev; the GOPRIVATE instructions and the internal issue-tracker link go away. Add a security policy pointing at GitHub's private vulnerability reporting, contributing guidelines, the Contributor Covenant 2.1, issue forms and a pull request template. The release workflow builds release notes from the changelog, so an Unreleased section records this pull request's changes; it becomes v0.2.0 at release.
6d0716b to
9375448
Compare
planadecu
left a comment
planadecu
left a comment
There was a problem hiding this comment.
Inline notes from a code review. The 429 / x-rate-limit-reset change reads as intended, and there's one question about the header format inline.
Bugs: partial-decode leakage across retries (client.go:415), an exact-limit body rejected as too large (client.go:274), and the bulk Params round-trip changing value types (metrics.go:101). Breaking changes are flagged for awareness only, since they need follow-up in glassnode-cli. These came from reading the code; none were reproduced by running it.
| return c.do(ctx, endpoint, params, func(body io.Reader) error { | ||
| decoder := json.NewDecoder(body) | ||
| decoder.UseNumber() | ||
| if err := decoder.Decode(dst); err != nil { |
There was a problem hiding this comment.
Bug: with streaming decode, a read failure partway through the body is retried into the same dst. encoding/json merges into existing maps and leaves struct fields alone when they're missing from the input. So if attempt 1 decodes keys A and B and then the connection resets, and attempt 2's body has only A, the caller gets a stale B from the failed attempt and no error.
The doc comment mentions this, but callers won't expect it. Suggest decoding each attempt into a fresh value, reflect.New(reflect.TypeOf(dst).Elem()), and copying it into dst only on success.
There was a problem hiding this comment.
Good catch, with one correction to the scenario: a cut in the middle of the value doesn't leak, because json.Decoder.Decode buffers the whole value before unmarshalling it — I tested that and dst stays empty. The leak is real one step later: a complete value followed by a failed read before EOF (e.g. Content-Length longer than the body) is a transport error, the retry decoded into the same dst, and the maps merged. Reproduced: attempt 1 {"a":1,"b":2} then cut, attempt 2 {"a":1} → result map[a:1 b:2].
Fixed differently from the reflect.New suggestion: the body is now read inside the retry loop and decoded once, after it, from the first successful attempt — the same approach stripe-go and go-github take. dst is written at most once, and a failed request or invalid JSON leaves it untouched. Since json.Decoder buffers the value anyway, the streaming decode saved no memory; it's dropped and the docs that claimed otherwise are corrected. Test added for your scenario.
planadecu
left a comment
planadecu
left a comment
There was a problem hiding this comment.
Re-review at f4e924b. All six fixes from the previous round hold up: decode once after the retry loop, ResponseTooLargeError, a body exactly at the limit is accepted, large reset values are read as timestamps, the Asset()/Network() accessors, and empty s is rejected. I didn't re-raise anything the replies already settled.
The new notes below are mostly edge cases in TokenRefresher and GetMetricCSV, and none of them block. They come from reading the code; none were reproduced by running it.
For glassnode-cli: entry.Asset used as a value ("a": entry.Asset) still compiles but stores a func, so JSON output fails at runtime. Both uses need (). CLI PR #6 already handles this.
Review round 1: - Decode once, after the retry loop, so a failed attempt never leaves values in the caller's destination; the streaming decode is dropped since json.Decoder buffers a whole value anyway. - A Retry-After or x-rate-limit-reset value above a year is read as a Unix timestamp, so a format change cannot silently disable retries. - ResponseTooLargeError replaces ErrResponseTooLarge-in-TransportError; a body of exactly the limit is accepted however it is chunked. - BulkEntry keeps every selector in Params with Asset() and Network() accessors; selectors are documented as strings; a is not required. - CHANGELOG starts from a single initial-release entry. Features requested in review: - GetMetricCSV streams a metric's CSV to an io.Writer; bulk is rejected, Content-Type is checked, writer errors are the caller's and a failure before any output is retried. - TokenRefresher lets a TokenSource replace a token the API rejected; one refresh per rejected token across goroutines, repeated once. Review round 2: - copyLimited holds the size-limit rule once; the writer never receives more than the limit. - HTML error pages are summarised by title (status-text fallback, entity decoding) instead of quoting markup; f=CSV matches case-insensitively. - Docs: the per-attempt timeout covers the body read; examples use the API key only.
d138c1e to
778245e
Compare
Summary
Aligns the client with the Glassnode API documentation and rewrites the project documentation.
API alignment
e,network,category, ...), butBulkEntrykept onlyaandnetwork, so entries for different exchanges or object-metric categories were indistinguishable.BulkEntry.Paramsnow holds all of them (Asset()/Network()read it), and the entry encodes back to the wire format.Since. The API requiressfor bulk requests;GetBulkMetricnow rejects a request without it before spending a call. The godoc and README describe the per-request time-range limits and credit cost.x-rate-limit-reset. The API reports its per-minute rate limit throughx-rate-limit-reset, notRetry-After. Retrying after 0-1s/0-2s only hit the limit again. A 429 now uses that header as the minimum delay (or returns immediately if it exceedsMaxDelay), and the wait is exposed asAPIError.RetryAfter.parameters_defaultsaccepts strings. The docs describe the values as strings, recorded responses use lists; both are accepted now instead of failing the whole metadata call.ListExchanges,ListNetworks,ListMinersfor the documented metadata endpoints.Findings of a shape audit over all 1818 metrics (run against the live API)
ComputedAton time-series points. Every point-in-time metric (843 of 1818) addscomputed_atto each point;TimeSeriesPointandObjectTimeSeriesPointdropped it. Two real PIT responses are added as contract fixtures.x-rate-limit-resetgoes up to 60; with a 30s cap the default client returned a 429 from the first half of a window without retrying at all.WithMaxResponseBytes. No size limit by default (as in go-github, stripe-go, slack-go); callers can opt into a hard bound, which fails with*ResponseTooLargeError(unwrapping toErrResponseTooLarge) and is not retried. The body is read inside the retry loop and decoded once, after it, so a failed attempt never leaves values in the caller's destination.GetAPIUsageaccepts accounts without add-ons (apiAddonsnull or absent).CSV downloads
GetMetricCSV(ctx, path, params, io.Writer)requests a metric withf=csvand copies the response to the writer as it arrives (the API serves regular metrics as CSV:timestamp,value, object keys,computed_atfor PIT; bulk is not available in CSV and is rejected before a request). Failures before the body are retried as usual; a failure during the copy is never retried, since part of the output is already written.OAuth refresh
TokenRefresher: aTokenSourcethat also implementsRefreshis asked once for a new token after a 401, and the request is repeated with it outside the retry budget. Concurrent calls rejected with the same token share one refresh (mutex, as ingolang.org/x/oauth2). An empty refreshed token is anAuthError; an unchanged one yields the 401. Lets glassnode-cli drop its refresh-on-401 transport.Review round 2 follow-ups
GetMetricCSV: sharedcopyLimitedso the writer never receives more thanWithMaxResponseBytes; a read failure before any output is retried, a writer failure is returned as the caller's error wrapped (never aTransportError); godoc and README note that the per-attempt timeout covers the body read.http.StatusTextfallback and entity decoding instead of quoting markup;f=CSVis matched case-insensitively.Next step (not in this PR): a per-point streaming decode API for full-history responses;
json.Decoderbuffers a whole value, so bufferedGetand stream-decode have the same peak memory.Other code fixes
WithUserAgentrejects any control character. Previously only CR/LF were rejected, so e.g."app\x00"built a client whose every request failed innet/httpand was retried as aTransportError.(*Client).redacthelper.Documentation
GOPRIVATEcommand, an accurate backoff description, a defaults table, a link to API key setup, a real CI badge and gofmt-formatted snippets. OAuth is described as "bring an existing token", since the client has no login flow.docs/design.mdturned from an implementation spec into design notes; the glassnode-cli migration plan is dropped (belongs in GN-159).v0.1.0was a private tag nobody consumed, so behaviour changes relative to it are noted in the review threads, not the changelog.CI
vX.Y.Ztag runs the tests and creates a GitHub release from that version's CHANGELOG section.Test plan
go test -race ./...,go vet ./...,gofmt -l .clean; examples module vets and builds; each commit tested on its ownSince, 429/503 withx-rate-limit-reset, string and listparameters_defaults, new list methods, invalid user agentsnetworkinParamsGetMetricCSV: streamed body, 503→200 retry without duplicate output, bulk/nil-writer/JSON-body rejection, no retry after partial output, size limit; smoke-tested against the live API<title>401</title>and entities;f=CSVAcceptx-rate-limit-reset, decode-once (no partial-decode leakage), chunked exact-limit body,ResponseTooLargeError, numeric selector round trip, emptyscomputed_atfixtures, usage without add-ons, interrupted body retried, invalid JSON not retried,WithMaxResponseBytes(Get, Raw, exact limit, negative), defaultMaxDelay≥ 60sgolangci-lint runandgovulncheckclean locally; workflows pass actionlint