refactor: standardize ingest http handler [OM-87] #3675
Conversation
This allows us to use the same features/approach/tooling to the ingest endpoint as the other handlers.
📝 WalkthroughWalkthroughThis PR refactors the ingest service architecture by converting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
openmeter/ingest/httpdriver/errors.go (1)
11-49: Error types + encoder look good; just a couple of small nitsThe overall shape (typed errors +
errorEncoderdelegating tocommonhttp.HandleErrorIfTypeMatches) is clean and consistent.A couple of tiny polish ideas you might consider:
- In
ErrorInvalidEvent.Error()/.Message(), callinge.Err.Error()will panic ifErris ever nil. Callers are currently always passing a non-nil error, but a quick guard (or formatting viafmt.Sprintf("invalid event: %v", e.Err)) would make this type safer against future misuse.Error()andMessage()currently duplicate the same string; ifcommonhttpdoesn’t require them to differ, you could have one delegate to the other to keep them from drifting.- The commented-out old return lines can probably be dropped now to keep the file tidy.
None of these are blockers; the behavior as written is fine.
openmeter/ingest/httpdriver/handler.go (1)
22-47: Double‑check semantics for missing namespace → 500
resolveNamespaceturns a missing namespace into:return "", commonhttp.NewHTTPError( http.StatusInternalServerError, errors.New("internal server error"), )If
GetNamespacecan fail because of user input (e.g. bad path/host/tenant), that probably wants to be a 4xx (400/404/422) so clients get a “you mis‑addressed this” signal instead of a 500.If, on the other hand,
GetNamespaceonly returnsok == falsewhen wiring/config is broken, then 500 is exactly right—just worth confirming that’s the case.You might also consider reusing a shared/internal error value instead of duplicating the
"internal server error"string here to keep things consistent with the rest of your error surface.openmeter/ingest/httpdriver/ingest.go (1)
94-107: Bool return fromService.IngestEventsis currently unusedIn the handler you do:
_, err := h.service.IngestEvents(ctx, params) if err != nil { return IngestEventsResponse{}, err }Given
Service.IngestEventsreturns(bool, error)and the implementation inservice.goalways returnstruewhenerr == nil, that boolean isn’t adding anything at the moment.Totally fine to ship as‑is, but worth aligning on intent:
- If you don’t foresee partial‑success semantics, consider simplifying the interface to just
error.- If you do plan to use the bool (e.g. “accepted vs. dropped”), you might eventually want to thread it into the HTTP response (status code or header) instead of ignoring it.
For now this is just a small design smell, not a blocker.
openmeter/ingest/service.go (2)
17-51: Config + constructor look good; consider reusing shared validation helperThe
Serviceinterface andConfig+NewServicepattern look clean and make wiring much nicer than constructing a concrete type directly.Minor optional tweak:
Config.Validateis hand‑rolling error aggregation witherrors.Join, whereas the repo already has a generic validator helper inpkg/models/validator.go. If you want consistency across services, you could delegate to that helper instead of re‑implementing the pattern here.Not a big deal either way; current code is totally fine.
58-96: Event processing is straightforward; a couple of small readability/design tweaksThe sequential loop over
request.Eventsand the timestamp normalization + logging beforecollector.Ingestall look good and easy to reason about.Two small things you might want to polish:
- The parameter name in
processEvent(event event.Event) shadows the importedeventpackage, which can be a bit confusing when scanning the function. Renaming the param toev(and, if desired, aliasing the import) would make it clearer.- As noted in the HTTP handler,
IngestEvents’sboolreturn is alwaystrueon success and doesn’t currently drive any behavior. If you don’t expect partial-success semantics, simplifying toerrorwould reduce a bit of noise; if you do, you might eventually want to expose that signal somewhere.Functionally this all looks sound; these are just small cleanup ideas.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/common/openmeter_server.go(1 hunks)cmd/server/main.go(1 hunks)cmd/server/wire.go(1 hunks)cmd/server/wire_gen.go(1 hunks)openmeter/ingest/httpdriver/errors.go(1 hunks)openmeter/ingest/httpdriver/handler.go(1 hunks)openmeter/ingest/httpdriver/ingest.go(1 hunks)openmeter/ingest/httpdriver/ingest_test.go(6 hunks)openmeter/ingest/ingestdriver/http_transport.go(0 hunks)openmeter/ingest/service.go(3 hunks)openmeter/server/router/event.go(1 hunks)openmeter/server/router/router.go(5 hunks)openmeter/server/server_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- openmeter/ingest/ingestdriver/http_transport.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
cmd/server/main.gocmd/server/wire.goopenmeter/ingest/httpdriver/errors.goapp/common/openmeter_server.goopenmeter/server/server_test.goopenmeter/server/router/event.goopenmeter/server/router/router.goopenmeter/ingest/httpdriver/handler.goopenmeter/ingest/httpdriver/ingest.gocmd/server/wire_gen.goopenmeter/ingest/service.goopenmeter/ingest/httpdriver/ingest_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/server/server_test.goopenmeter/ingest/httpdriver/ingest_test.go
🧠 Learnings (1)
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/server/server_test.goopenmeter/server/router/router.go
🧬 Code graph analysis (10)
cmd/server/wire.go (1)
openmeter/ingest/service.go (1)
Service(13-15)
openmeter/ingest/httpdriver/errors.go (1)
pkg/framework/commonhttp/errors.go (1)
HandleErrorIfTypeMatches(63-76)
app/common/openmeter_server.go (1)
openmeter/ingest/service.go (3)
Service(13-15)NewService(42-51)Config(17-20)
openmeter/server/server_test.go (1)
openmeter/ingest/service.go (2)
Service(13-15)IngestEventsRequest(53-56)
openmeter/server/router/router.go (1)
openmeter/ingest/service.go (1)
Service(13-15)
openmeter/ingest/httpdriver/handler.go (5)
openmeter/server/router/router.go (1)
IngestHandler(85-87)openmeter/ingest/httpdriver/ingest.go (1)
IngestEventsHandler(20-20)openmeter/ingest/service.go (1)
Service(13-15)pkg/framework/transport/httptransport/options.go (1)
HandlerOption(19-21)pkg/framework/commonhttp/errors.go (1)
NewHTTPError(36-42)
openmeter/ingest/httpdriver/ingest.go (6)
openmeter/ingest/service.go (1)
IngestEventsRequest(53-56)openmeter/ingest/httpdriver/handler.go (2)
Handler(14-16)New(37-47)app/common/namespace.go (1)
Namespace(13-15)api/api.gen.go (3)
IngestEventsBody(3835-3837)IngestEventsApplicationCloudeventsPlusJSONRequestBody(9354-9354)IngestEventsApplicationCloudeventsBatchPlusJSONBody(8503-8503)openmeter/ingest/httpdriver/errors.go (2)
ErrorInvalidEvent(31-33)ErrorInvalidContentType(11-13)pkg/framework/commonhttp/encoder.go (1)
EmptyResponseEncoder(120-125)
cmd/server/wire_gen.go (1)
openmeter/ingest/service.go (1)
Service(13-15)
openmeter/ingest/service.go (3)
openmeter/ingest/httpdriver/ingest.go (1)
IngestEventsRequest(18-18)openmeter/ingest/kafkaingest/collector.go (1)
Collector(30-39)pkg/models/validator.go (1)
Validate(16-26)
openmeter/ingest/httpdriver/ingest_test.go (2)
openmeter/ingest/service.go (2)
NewService(42-51)Config(17-20)openmeter/ingest/httpdriver/handler.go (1)
New(37-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
openmeter/ingest/httpdriver/ingest_test.go (3)
23-70: LGTM! Clean migration to the service-based pattern.The test correctly uses the new factory pattern with
ingest.NewServiceand wires throughhttpdriver.New. Error handling is solid, and the test coverage is comprehensive.
72-95: Good error case coverage.The test properly validates that invalid events return 400 Bad Request.
97-149: Excellent batch test coverage.The batch handling test is thorough, creating 10 events and verifying they're all properly ingested.
openmeter/server/router/event.go (1)
10-12: Nice! Consistent with the handler pattern.The change to
a.ingestHandler.IngestEvents().ServeHTTP(w, r)aligns nicely with how other handlers in this file work (likea.meterEventHandler.ListEvents()on line 16).cmd/server/main.go (1)
142-178: Clean separation of concerns.Moving from
IngestHandlertoIngestServicehere is great—it pushes the HTTP transport concerns down to the router layer where they belong, keeping the main app wiring focused on business logic.cmd/server/wire.go (1)
59-59: Interface value semantics—perfect.Changing from
*ingest.Servicetoingest.Serviceis the right move sinceServiceis an interface. This is more idiomatic and aligns with Go best practices.cmd/server/wire_gen.go (1)
649-649: Consistent with wire.go.The generated code correctly reflects the interface value type change.
openmeter/server/router/router.go (4)
104-104: Service-based configuration—nice and clean.Adding
IngestService ingest.Serviceto the config follows the same pattern as other services in this struct.
132-134: Validation in place.The nil check for
IngestServicematches the validation pattern for other services in this function.
239-239: Handler field added to Router.The
ingestHandler ingesthttpdriver.Handlerfield follows the naming and type pattern of other handlers in this struct (likemeterHandler,customerHandler, etc.).
301-305: Handler initialization consistent with the codebase pattern.The initialization of
ingestHandlerusingingesthttpdriver.Newwith the namespace decoder, service, and error handler mirrors how other handlers are created (compare withmeterHandleron lines 292-299).openmeter/server/server_test.go (2)
1594-1604: Clean noop implementation for testing.The
NoopIngestServicefollows the same pattern as other noop services in this file (likeNoopCustomerService,NoopBillingService, etc.). The type assertion on line 1596 ensures compile-time verification of interface compliance, which is a nice touch.
521-522: Test wiring updated correctly.Replacing the handler with the service in the test config aligns with the refactored architecture.
app/common/openmeter_server.go (1)
107-115: Factory pattern with validation—excellent.Switching to
ingest.NewServiceis great because it centralizes service construction and validation logic. The return type change toingest.Service(value) is correct for interfaces.
| contentType := r.Header.Get("Content-Type") | ||
|
|
||
| switch contentType { | ||
| case "application/json": | ||
| var apiRequest api.IngestEventsBody | ||
|
|
||
| err := json.NewDecoder(r.Body).Decode(&apiRequest) | ||
| if err != nil { | ||
| return req, ErrorInvalidEvent{ | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| // Try to parse as a single event | ||
| e, err := apiRequest.AsEvent() | ||
| if err == nil { | ||
| req.Events = []event.Event{e} | ||
| } else { | ||
| // Try to parse as a batch of events | ||
| e, err := apiRequest.AsIngestEventsBody1() | ||
| if err == nil { | ||
| req.Events = e | ||
| } | ||
| } | ||
|
|
||
| // If we still don't have any events, return an error | ||
| if len(req.Events) == 0 { | ||
| return req, ErrorInvalidEvent{ | ||
| Err: errors.New("no events found"), | ||
| } | ||
| } | ||
| case "application/cloudevents+json": | ||
| var apiRequest api.IngestEventsApplicationCloudeventsPlusJSONRequestBody | ||
|
|
||
| err := json.NewDecoder(r.Body).Decode(&apiRequest) | ||
| if err != nil { | ||
| return req, ErrorInvalidEvent{ | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| req.Events = []event.Event{apiRequest} | ||
| case "application/cloudevents-batch+json": | ||
| var apiRequest api.IngestEventsApplicationCloudeventsBatchPlusJSONBody | ||
|
|
||
| err := json.NewDecoder(r.Body).Decode(&apiRequest) | ||
| if err != nil { | ||
| return req, ErrorInvalidEvent{ | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| req.Events = apiRequest | ||
| default: | ||
| return req, ErrorInvalidContentType{ContentType: contentType} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "ingest.go" | grep httpdriverRepository: openmeterio/openmeter
Length of output: 102
🏁 Script executed:
cat -n openmeter/ingest/httpdriver/ingest.go | sed -n '1,100p'Repository: openmeterio/openmeter
Length of output: 3311
🏁 Script executed:
rg "IngestEvents" -A 5 --type go | grep -A 5 "func.*IngestEvents.*bool"Repository: openmeterio/openmeter
Length of output: 881
🏁 Script executed:
fd -e go | xargs rg -l "IngestEvents" | head -5Repository: openmeterio/openmeter
Length of output: 218
🏁 Script executed:
rg "type.*IngestEvents" --type go -B 2 -A 5 | head -30Repository: openmeterio/openmeter
Length of output: 1511
🏁 Script executed:
rg "interface.*Ingest" --type go -B 2 -A 10Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
python3 << 'EOF'
import subprocess
result = subprocess.run(['python3', '-c', 'import net.mime; print("net.mime available")'],
capture_output=True, text=True)
# Just check if mime.ParseMediaType is in Go stdlib
print("Checking Go mime.ParseMediaType location...")
# In Go stdlib, mime.ParseMediaType is at net/mime package
print("Go stdlib: mime.ParseMediaType is in 'net/mime' package")
EOFRepository: openmeterio/openmeter
Length of output: 151
🏁 Script executed:
rg "type.*Service.*interface" openmeter/ingest --type go -A 15Repository: openmeterio/openmeter
Length of output: 829
Content‑Type handling is too strict and will reject real-world HTTP clients
The current code switches on the raw Content-Type header string:
contentType := r.Header.Get("Content-Type")
switch contentType {
case "application/json":This will reject valid requests with application/json; charset=utf-8 (or similar with additional parameters), causing them to hit the default branch and fail with ErrorInvalidContentType. Since this is on the event ingestion hot path, this is a real correctness issue.
Use mime.ParseMediaType from the standard library to normalize:
import "net/mime"
mediaType, _, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
if err != nil {
return req, ErrorInvalidContentType{ContentType: r.Header.Get("Content-Type")}
}
switch mediaType {
case "application/json":Additionally, improve error diagnostics in the JSON union parsing case (lines 49–65). When both AsEvent() and AsIngestEventsBody1() fail, the code currently returns a generic errors.New("no events found"), which discards the actual parsing error. Capturing and returning the last non-nil error from those helpers would significantly improve debuggability for clients and operators.
Overview
This allows us to use the same features/approach/tooling to the ingest endpoint as the other handlers.
Tests are passing, validated manually that ingest works.
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.