feat(sdk/go): add baseline hand-written Go SDK for v3 meters#4644
feat(sdk/go): add baseline hand-written Go SDK for v3 meters#4644gergely-kurucz-konghq wants to merge 2 commits into
Conversation
Hand-written, idiomatic Go SDK as a reference shape for a future TypeSpec emitter. Standalone module (github.com/openmeterio/openmeter/sdk/go). - net/http public surface; go-retryablehttp hidden behind *http.Client - resource-grouped ops: Meters.Get / List / Query / QueryCSV - query-string serialization (deepObject page/filter, form sort) - typed APIError; bearer auth set at request-build time (survives client injection) - unit tests via httptest + env-gated TestLive (verified vs local and dev cloud)
gergely-kurucz-konghq
added
kind/feature
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Assert filter[key][eq] round-trips through a real server: exact-match returns the meter, a bogus key returns none.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (s *MetersService) Get(ctx context.Context, meterID string) (*Meter, error) { | ||
| req, err := s.client.newRequest(ctx, http.MethodGet, metersBasePath+"/"+url.PathEscape(meterID), nil, nil, contentTypeJSON) |
There was a problem hiding this comment.
meterID validation
Get(ctx, "") builds the path /openmeter/meters/ (note trailing slash). Depending on the server's router, that either silently maps to the List endpoint—doJSON then decodes the paginated list envelope {"data":[...],"meta":{...}} into a Meter struct, which succeeds but returns a zero-value struct with no error—or returns a 4xx whose message ("Not Found", etc.) gives no hint the caller forgot to pass an ID. The same latent issue exists in Query and QueryCSV. An upfront check eliminates the ambiguity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/go/meters.go
Line: 82-83
Comment:
**Missing empty `meterID` validation**
`Get(ctx, "")` builds the path `/openmeter/meters/` (note trailing slash). Depending on the server's router, that either silently maps to the List endpoint—`doJSON` then decodes the paginated list envelope `{"data":[...],"meta":{...}}` into a `Meter` struct, which succeeds but returns a zero-value struct with no error—or returns a 4xx whose message ("Not Found", etc.) gives no hint the caller forgot to pass an ID. The same latent issue exists in `Query` and `QueryCSV`. An upfront check eliminates the ambiguity.
How can I resolve this? If you propose a fix, please make it concise.
| // Dimensions maps group-by dimension names to JSONPath expressions. | ||
| Dimensions *map[string]string `json:"dimensions,omitempty"` | ||
| Labels *map[string]string `json:"labels,omitempty"` |
There was a problem hiding this comment.
map is already a reference type in Go: a nil map is the idiomatic "not set" value and is correctly omitted by json:",omitempty". Wrapping it in an extra pointer forces callers to double-dereference and breaks the clean "nil = absent" mental model. Replacing *map[string]string with plain map[string]string gives the same JSON serialization behaviour with no change in correctness.
| // Dimensions maps group-by dimension names to JSONPath expressions. | |
| Dimensions *map[string]string `json:"dimensions,omitempty"` | |
| Labels *map[string]string `json:"labels,omitempty"` | |
| // Dimensions maps group-by dimension names to JSONPath expressions. | |
| Dimensions map[string]string `json:"dimensions,omitempty"` | |
| Labels map[string]string `json:"labels,omitempty"` |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/go/models.go
Line: 36-38
Comment:
**Pointer-to-map is unconventional and complicates the caller**
`map` is already a reference type in Go: a nil map is the idiomatic "not set" value and is correctly omitted by `json:",omitempty"`. Wrapping it in an extra pointer forces callers to double-dereference and breaks the clean "nil = absent" mental model. Replacing `*map[string]string` with plain `map[string]string` gives the same JSON serialization behaviour with no change in correctness.
```suggestion
// Dimensions maps group-by dimension names to JSONPath expressions.
Dimensions map[string]string `json:"dimensions,omitempty"`
Labels map[string]string `json:"labels,omitempty"`
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // MeterQueryFilters filters a meter query by dimension values. | ||
| type MeterQueryFilters struct { | ||
| Dimensions *map[string]QueryFilterStringMapItem `json:"dimensions,omitempty"` | ||
| } |
There was a problem hiding this comment.
Meter.Dimensions/Labels. A plain map[string]QueryFilterStringMapItem is nil-safe, already a reference type, and omitted by omitempty when nil—no pointer needed.
| // MeterQueryFilters filters a meter query by dimension values. | |
| type MeterQueryFilters struct { | |
| Dimensions *map[string]QueryFilterStringMapItem `json:"dimensions,omitempty"` | |
| } | |
| // MeterQueryFilters filters a meter query by dimension values. | |
| type MeterQueryFilters struct { | |
| Dimensions map[string]QueryFilterStringMapItem `json:"dimensions,omitempty"` | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/go/models.go
Line: 85-88
Comment:
Same pointer-to-map concern as `Meter.Dimensions`/`Labels`. A plain `map[string]QueryFilterStringMapItem` is nil-safe, already a reference type, and omitted by `omitempty` when nil—no pointer needed.
```suggestion
// MeterQueryFilters filters a meter query by dimension values.
type MeterQueryFilters struct {
Dimensions map[string]QueryFilterStringMapItem `json:"dimensions,omitempty"`
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -0,0 +1,7 @@ | |||
| module github.com/openmeterio/openmeter/sdk/go | |||
There was a problem hiding this comment.
The last segment of github.com/openmeterio/openmeter/sdk/go is the reserved keyword go, so import "github.com/openmeterio/openmeter/sdk/go" is a compile-time error. Every consumer must write import openmeter "github.com/openmeterio/openmeter/sdk/go". The README calls this out, but it is a persistent ergonomic friction point. A module path like github.com/openmeterio/openmeter/sdk/go/openmeter (with a nested package) would let callers use the natural package name without an alias.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/go/go.mod
Line: 1
Comment:
**Module path forces a mandatory import alias**
The last segment of `github.com/openmeterio/openmeter/sdk/go` is the reserved keyword `go`, so `import "github.com/openmeterio/openmeter/sdk/go"` is a compile-time error. Every consumer must write `import openmeter "github.com/openmeterio/openmeter/sdk/go"`. The README calls this out, but it is a persistent ergonomic friction point. A module path like `github.com/openmeterio/openmeter/sdk/go/openmeter` (with a nested package) would let callers use the natural package name without an alias.
How can I resolve this? If you propose a fix, please make it concise.| func defaultHTTPClient() *http.Client { | ||
| rc := retryablehttp.NewClient() | ||
| rc.RetryMax = 3 | ||
| rc.RetryWaitMin = 500 * time.Millisecond | ||
| rc.RetryWaitMax = 5 * time.Second | ||
| rc.Logger = nil // silence the default stdout logger | ||
| return rc.StandardClient() | ||
| } |
There was a problem hiding this comment.
retryablehttp's default CheckRetry policy retries any HTTP method—including POST—on 5xx responses and connection errors. The current POST endpoint (/meters/{id}/query) is a read, so this is safe today. However, if the SDK later grows write operations and callers rely on the default transport, duplicate side effects can occur silently on retried 5xx responses. Consider overriding CheckRetry to restrict automatic retries to idempotent methods (GET, HEAD), or at least document the retry-on-POST behaviour prominently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/go/transport.go
Line: 24-31
Comment:
**Default client retries POST requests on 5xx**
`retryablehttp`'s default `CheckRetry` policy retries any HTTP method—including POST—on 5xx responses and connection errors. The current POST endpoint (`/meters/{id}/query`) is a read, so this is safe today. However, if the SDK later grows write operations and callers rely on the default transport, duplicate side effects can occur silently on retried 5xx responses. Consider overriding `CheckRetry` to restrict automatic retries to idempotent methods (`GET`, `HEAD`), or at least document the retry-on-POST behaviour prominently.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
What
Adds a baseline hand-written Go SDK for the OpenMeter v3 API as a new standalone module (
github.com/openmeterio/openmeter/sdk/go). Implements the three meter endpoints —Get,List,Query(+QueryCSV) — as a worked reference of the target SDK shape.Why
The existing generated client (
api/client/go) is a 1.7MB oapi-codegen monolith targeting the v1 cloud spec. We want an idiomatic, dependency-light SDK whose shape can later be reproduced by a TypeSpec emitter. This PR establishes that gold-standard shape against real endpoints before automating it.How
net/httppublic surface — no third-party type leaks.hashicorp/go-retryablehttpprovides retries but is fully hidden behind the*http.Clientseam (StandardClient()), soWithHTTPClientswaps transport cleanly.client.Meters.Get / List / Query / QueryCSV.list-meters: deepObjectpage, formsort, nested deepObjectfilter.APIError— decodes the RFC7807 problem body (status, title, detail, correlation ID).*http.Client.DateTime→time.Time,Numeric→string, pagination counts→int).Verification: unit tests via
httptest(no network) + an env-gatedTestLive. Verified against a local server (unauthenticated) andhttps://dev.openmeter.cloud/api/v3(bearer token) — success, auth, and 401 error paths all confirmed.Note: standalone module, not in any
go.work; the maingo build ./...does not cover it. CI wiring is a follow-up.