feat(metrics): add per-IP logs and DeFi transaction content metrics#440
feat(metrics): add per-IP logs and DeFi transaction content metrics#440
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an internal/metrics package with Prometheus collectors and recording APIs, and a submit.ParseTxInfo function plus submit.TxInfo type to extract transaction content signals (script type, minting, reference inputs). Updates the API handler to call metrics.Register(), compute client IP via a new realClientIP helper (honoring ApiConfig.TrustedProxies), and record request/result and content metrics and success/fail counts via the metrics package. Adds unit tests for metrics, realClientIP, and tx parsing. go.mod dependency versions were bumped and an indirect dependency on github.com/kylelemons/godebug was added. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/metrics/metrics.go">
<violation number="1" location="internal/metrics/metrics.go:105">
P1: Using raw client IP as a Prometheus label creates unbounded cardinality and allows metric-series explosion from user-controlled inputs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // RecordTxRequest records a submission attempt. result is one of "accepted", | ||
| // "rejected" (node rejected the tx), or "error" (parse or internal error). | ||
| func RecordTxRequest(ip, result string) { | ||
| txSubmitRequestsTotal.WithLabelValues(ip, result).Inc() |
There was a problem hiding this comment.
P1: Using raw client IP as a Prometheus label creates unbounded cardinality and allows metric-series explosion from user-controlled inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/metrics/metrics.go, line 105:
<comment>Using raw client IP as a Prometheus label creates unbounded cardinality and allows metric-series explosion from user-controlled inputs.</comment>
<file context>
@@ -0,0 +1,132 @@
+// RecordTxRequest records a submission attempt. result is one of "accepted",
+// "rejected" (node rejected the tx), or "error" (parse or internal error).
+func RecordTxRequest(ip, result string) {
+ txSubmitRequestsTotal.WithLabelValues(ip, result).Inc()
+}
+
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
submit/txinfo_test.go (1)
49-123: Cover the remaining script-type branches.The parser has explicit
nativeandplutus_v2outputs, but this suite only asserts"none","plutus_v1", and"plutus_v3". A typo or precedence regression in either untested branch would skew the new metrics without failing CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@submit/txinfo_test.go` around lines 49 - 123, Add tests covering the missing "native" and "plutus_v2" branches by adding two new parallel test functions (e.g. TestParseTxInfo_NativeScript and TestParseTxInfo_PlutusV2) that call ParseTxInfo with the existing fixture hex blobs (nativeScriptTxHex and plutusV2TxHex via mustDecodeHex), assert info.ScriptType equals "native" and "plutus_v2" respectively, and assert the expected HasMinting and HasReferenceInputs booleans for those fixtures (match the fixtures' expected values, e.g. typically false if they don't include minting/ref inputs); keep the same error handling pattern used in the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/api.go`:
- Around line 372-380: The tx-content parsing using submit.ParseTxInfo should be
best-effort, not a hard validation gate; change the error branch so that on
parse failure you log the error (include err and context such as clientIP),
increment any relevant metrics for a parse failure if desired, set txInfo to a
nil/empty value or a safe default, and continue the normal submission flow
instead of calling writeJSON(...) and returning; update the same pattern in the
other block around lines where txInfo is parsed (also references to
metrics.IncTxSubmitFailCount and metrics.RecordTxRequest) so parsing failures
only affect the content metric and do not abort the request.
- Around line 344-345: The code currently sets clientIP := realClientIP(r)
trusting X-Real-IP/X-Forwarded-For from any caller; change realClientIP handling
so it only honors forwarded headers when the immediate peer (r.RemoteAddr or
parsed remote IP) is in the configured trusted proxies list, otherwise fall back
to using r.RemoteAddr (or the parsed remote IP) as the client IP; update the
logic used wherever realClientIP is called (including the similar block around
the code handling lines 440-456) to validate the immediate peer against the
trusted proxies set before parsing X-Forwarded-For/X-Real-IP and ensure metrics
use the resulting non-spoofable clientIP variable.
In `@internal/metrics/metrics.go`:
- Around line 17-21: The collectors are currently lazily created in Register()
causing IncTxSubmitCount() and IncTxSubmitFailCount() to panic if called earlier
and Register()/Start() to panic on duplicate calls; fix by eagerly initializing
all prometheus collectors in package init() (so globals used by
IncTxSubmitCount/IncTxSubmitFailCount are always non-nil) and protect the
prometheus.MustRegister(...) path with a sync.Once (e.g., a package-level var
registerOnce sync.Once used inside Register/Start) to make registration
idempotent and safe on repeated Start() calls.
---
Nitpick comments:
In `@submit/txinfo_test.go`:
- Around line 49-123: Add tests covering the missing "native" and "plutus_v2"
branches by adding two new parallel test functions (e.g.
TestParseTxInfo_NativeScript and TestParseTxInfo_PlutusV2) that call ParseTxInfo
with the existing fixture hex blobs (nativeScriptTxHex and plutusV2TxHex via
mustDecodeHex), assert info.ScriptType equals "native" and "plutus_v2"
respectively, and assert the expected HasMinting and HasReferenceInputs booleans
for those fixtures (match the fixtures' expected values, e.g. typically false if
they don't include minting/ref inputs); keep the same error handling pattern
used in the other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecd81817-9090-45db-8eb1-feedd0f853d4
📒 Files selected for processing (7)
go.modinternal/api/api.gointernal/api/api_test.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gosubmit/txinfo.gosubmit/txinfo_test.go
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
6 times, most recently
from
69b3106 to
1a67d01
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/api/api.go (2)
344-344: Consider cardinality controls for per-IP metrics in production.The
tx_submit_requests_total{ip, result}counter creates a new time series for each unique client IP. In high-traffic environments with many distinct clients, this can lead to:
- Unbounded label cardinality
- Prometheus memory exhaustion
- Slow queries and increased storage costs
Mitigation options for production:
- Apply IP aggregation (e.g., /24 subnets instead of individual IPs)
- Use metric relabeling to drop/limit the
iplabel at scrape time- Set
sample_limitin Prometheus scrape config- Monitor
prometheus_tsdb_symbol_table_size_bytesfor early warningAlso applies to: 368-368, 411-411, 421-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` at line 344, The per-request metric uses the raw client IP (clientIP := realClientIP(...)) as a label which risks unbounded label cardinality for the tx_submit_requests_total counter and related metrics; update the code to aggregate or normalize client IPs before using them as labels (e.g., compute an aggregatedIP by masking IPv4 to a /24 via net.IP.Mask(net.CIDRMask(24,32)) and apply an equivalent aggregation for IPv6, or fall back to a fixed token like "redacted" when unknown) and replace uses of clientIP in the counter labels (where tx_submit_requests_total and the other mentioned counters are incremented) with aggregatedIP; alternatively add a config toggle to disable attaching the IP label so operators can turn it off in prod.
469-475: Consider validating forwarded header values as IP addresses.The
X-Real-IPandX-Forwarded-Forvalues are returned directly without validation. Even from trusted proxies, malformed values (e.g., hostnames, garbage strings) would flow into Prometheus labels, potentially causing misleading metrics or label cardinality issues.🛡️ Suggested validation
if inRange { if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" { + if net.ParseIP(ip) != nil { + return ip + } - return ip } if forwarded := r.Header.Get("X-Forwarded-For"); forwarded != "" { first, _, _ := strings.Cut(forwarded, ",") - return strings.TrimSpace(first) + first = strings.TrimSpace(first) + if net.ParseIP(first) != nil { + return first + } } break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` around lines 469 - 475, Validate X-Real-IP and the first X-Forwarded-For token as real IP addresses instead of returning raw header values: for the X-Real-IP branch, trim the header value and call net.ParseIP on it (using net.ParseIP and not accepting hostnames); if ParseIP returns non-nil return that, otherwise continue; for the X-Forwarded-For branch split on comma, trim the first token and net.ParseIP it and only return if non-nil; if neither header yields a valid IP, extract the host from r.RemoteAddr with net.SplitHostPort and net.ParseIP as a final fallback or return empty string—update the code around r.Header.Get("X-Real-IP"), r.Header.Get("X-Forwarded-For"), and r.RemoteAddr accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/api.go`:
- Line 344: The per-request metric uses the raw client IP (clientIP :=
realClientIP(...)) as a label which risks unbounded label cardinality for the
tx_submit_requests_total counter and related metrics; update the code to
aggregate or normalize client IPs before using them as labels (e.g., compute an
aggregatedIP by masking IPv4 to a /24 via net.IP.Mask(net.CIDRMask(24,32)) and
apply an equivalent aggregation for IPv6, or fall back to a fixed token like
"redacted" when unknown) and replace uses of clientIP in the counter labels
(where tx_submit_requests_total and the other mentioned counters are
incremented) with aggregatedIP; alternatively add a config toggle to disable
attaching the IP label so operators can turn it off in prod.
- Around line 469-475: Validate X-Real-IP and the first X-Forwarded-For token as
real IP addresses instead of returning raw header values: for the X-Real-IP
branch, trim the header value and call net.ParseIP on it (using net.ParseIP and
not accepting hostnames); if ParseIP returns non-nil return that, otherwise
continue; for the X-Forwarded-For branch split on comma, trim the first token
and net.ParseIP it and only return if non-nil; if neither header yields a valid
IP, extract the host from r.RemoteAddr with net.SplitHostPort and net.ParseIP as
a final fallback or return empty string—update the code around
r.Header.Get("X-Real-IP"), r.Header.Get("X-Forwarded-For"), and r.RemoteAddr
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38cf83f5-0886-4ba4-9720-cd4d30b9ef69
📒 Files selected for processing (8)
go.modinternal/api/api.gointernal/api/api_test.gointernal/config/config.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gosubmit/txinfo.gosubmit/txinfo_test.go
✅ Files skipped from review due to trivial changes (5)
- go.mod
- internal/metrics/metrics_test.go
- submit/txinfo_test.go
- submit/txinfo.go
- internal/metrics/metrics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/api_test.go
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
from
1a67d01 to
57b3542
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/api.go`:
- Around line 344-345: The code computes clientIP via realClientIP but never
passes it into the request counter; update the metrics shape and call sites to
include per-client-ip labels and thread clientIP through RecordTxRequest: add a
client_ip label to tx_submit_requests_total in internal/metrics/metrics.go,
change the RecordTxRequest function signature (and any wrapper helpers) to
accept a clientIP string and use it when incrementing the counter, then update
all callers in internal/api/api.go that call RecordTxRequest (the places that
compute clientIP and later call RecordTxRequest) to pass the computed clientIP,
and finally update internal/api/api_test.go assertions to expect the per-IP
series.
- Around line 469-475: The helper realClientIP should validate X-Real-IP and the
first token of X-Forwarded-For before returning them: parse the candidate string
using net.ParseIP and only return it when ParseIP yields a non-nil result;
otherwise fall back to returning peerHost. Update the logic in realClientIP (the
branch that reads r.Header.Get("X-Real-IP") and the branch that extracts the
first token from X-Forwarded-For) to trim space, parse with net.ParseIP, and
return peerHost when parsing fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fae69104-ae64-4ff8-8095-81cb39a7a6e2
📒 Files selected for processing (8)
go.modinternal/api/api.gointernal/api/api_test.gointernal/config/config.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gosubmit/txinfo.gosubmit/txinfo_test.go
✅ Files skipped from review due to trivial changes (3)
- go.mod
- submit/txinfo_test.go
- internal/metrics/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/config/config.go
- internal/metrics/metrics.go
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
from
57b3542 to
ae7126c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/api/api.go (2)
391-394: Redundant nil check ontxRejectErr.
errors.Asonly returnstrueafter setting its target to the matching error, sotxRejectErris guaranteed non-nil whenisRejectedis true. The&& txRejectErr != nilis dead-code defensive and adds noise.♻️ Cleanup
- if isRejected && txRejectErr != nil { + if isRejected { w.Header().Set("Content-Type", "application/cbor")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` around lines 391 - 394, The check redundantly tests txRejectErr for nil after using errors.As; remove the dead-code part so the condition reads just `if isRejected { ... }` (or directly use the boolean result of errors.As) when handling CBOR responses; update the branch that currently reads `if isRejected && txRejectErr != nil` to `if isRejected` and keep using txRejectErr inside the block (symbols: txRejectErr, errors.As, r.Header.Get("Accept")).
445-486: ParsetrustedProxiesonce at startup, and consider walking XFF right-to-left.Two observations on the helper:
Per-request CIDR parsing. Every request re-parses the
trustedProxiesslice (net.ParseCIDR/net.ParseIPper entry) inside the loop. For a hot path this is avoidable work. Consider parsing into[]net.IPNet/[]net.IPonce at startup (e.g., atStart()time) and passing the pre-parsed form into the handler, or caching it in a package var.Leftmost X-Forwarded-For. You take the first XFF entry as the client IP. In a chain
client, proxy_A, proxy_Bwhere onlyproxy_Bis trusted, the leftmost value is still the untrusted client's claim and is trivially spoofable by any HTTP client that sets its ownX-Forwarded-For. The safer convention is to walk XFF right-to-left, skipping entries whose IPs are themselves intrustedProxies, and return the first untrusted hop. This matches the behavior of nginx'sreal_ip_recursive onand most frameworks (e.g., Express'trust proxywith a numeric/function value). If the deployment only ever has a single trusted hop in front of this service, the current code is fine — worth a comment making that assumption explicit.Minor: on Line 459 the outer
errfromnet.SplitHostPort(Line 447) is reused as the target ofnet.ParseCIDR. Not a bug since the value is only checked locally, butvaror:=into a new local would be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api.go` around lines 445 - 486, The realClientIP helper currently reparses trustedProxies per request and naively returns the leftmost X-Forwarded-For value; fix it by pre-parsing trustedProxies at startup into a structure (e.g., []net.IPNet and []net.IP) and change realClientIP to accept that pre-parsed list instead of []string so it avoids net.ParseCIDR/net.ParseIP on each request, then implement XFF parsing right-to-left: split X-Forwarded-For, iterate from last to first, skip entries whose IPs are in the trusted set and return the first untrusted hop (falling back to X-Real-IP if appropriate and finally peerHost), and ensure you stop reusing the outer err variable (use new locals or var declarations) when calling net.ParseCIDR/net.ParseIP.internal/api/api_test.go (1)
289-326: Fragile coupling tohttptest.NewRequestdefaultRemoteAddr.Both tests rely on the undocumented-in-API fact that
httptest.NewRequestsetsRemoteAddrto"192.0.2.1:1234". If that default changes in a future Go release, both tests silently assert against the wrong series (beforeandafterwould both be 0 and the equality check might still pass by coincidence, or just fail opaquely).Consider setting
req.RemoteAddrexplicitly for clarity and isolation:♻️ Suggested tightening
func TestSubmitTx_RequestsTotal_InvalidCBOR(t *testing.T) { // Not parallel: reads counter value which is package-global state. - // httptest.NewRequest sets RemoteAddr = "192.0.2.1:1234". const clientIP = "192.0.2.1" before := testutil.ToFloat64(metrics.TxSubmitRequestsTotal().WithLabelValues(clientIP, "error")) rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, "/api/submit/tx", strings.NewReader("not-valid-cbor")) req.Header.Set("Content-Type", "application/cbor") + req.RemoteAddr = clientIP + ":1234" newTestMux().ServeHTTP(rec, req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/api_test.go` around lines 289 - 326, Both tests (TestSubmitTx_RequestsTotal_InvalidCBOR and TestSubmitTx_RequestsTotal_NoIncrementOnBadContentType) rely on httptest.NewRequest's default RemoteAddr; explicitly set req.RemoteAddr = "192.0.2.1:1234" after creating the request (before calling newTestMux().ServeHTTP) so the clientIP constant used for metrics.TxSubmitRequestsTotal().WithLabelValues(clientIP, ...) matches reliably and removes fragile coupling to httptest defaults.internal/metrics/metrics.go (1)
114-124: Minor: inconsistent nil-guarding across recording helpers.
IncTxSubmitCount/IncTxSubmitFailCount(Lines 100–110) guard against nil collectors, butRecordTxRequest/RecordTxContentdo not. Sinceinit()always runs before user code, in practice they cannot be nil, so either drop the guards from the Inc helpers for consistency, or add matching guards here. Leaning toward dropping the guards since the eagerinit()makes them unreachable.♻️ Proposed cleanup
func IncTxSubmitCount() { - if txSubmitCount != nil { - txSubmitCount.Inc() - } + txSubmitCount.Inc() } func IncTxSubmitFailCount() { - if txSubmitFailCount != nil { - txSubmitFailCount.Inc() - } + txSubmitFailCount.Inc() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/metrics/metrics.go` around lines 114 - 124, Remove the redundant nil-guards from the IncTxSubmitCount and IncTxSubmitFailCount helpers so behavior matches RecordTxRequest and RecordTxContent (which assume collectors are initialized in init()); specifically, eliminate the if <collector> != nil checks in the IncTxSubmitCount and IncTxSubmitFailCount functions (references: IncTxSubmitCount, IncTxSubmitFailCount, txSubmitRequestsTotal, txSubmitRequestsFailedTotal) and call .Inc() unguarded like the other record helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/metrics/metrics.go`:
- Around line 51-57: The txSubmitRequestsTotal metric currently uses an
unbounded "ip" label causing high-cardinality risk; remove or replace the ip
label in the prometheus.NewCounterVec call (txSubmitRequestsTotal) — e.g.,
change the label slice from []string{"ip","result"} to a bounded alternative
like []string{"result"} (or to a bucketed label such as "ip_prefix" if you
implement an IP /24 or /64 bucketing function), update any places that call
txSubmitRequestsTotal.WithLabelValues(...) to only pass the new labels, and
adjust tests (internal/api/api_test.go expectations that reference "192.0.2.1")
to match the new label scheme or verify per-IP data via logs instead.
---
Nitpick comments:
In `@internal/api/api_test.go`:
- Around line 289-326: Both tests (TestSubmitTx_RequestsTotal_InvalidCBOR and
TestSubmitTx_RequestsTotal_NoIncrementOnBadContentType) rely on
httptest.NewRequest's default RemoteAddr; explicitly set req.RemoteAddr =
"192.0.2.1:1234" after creating the request (before calling
newTestMux().ServeHTTP) so the clientIP constant used for
metrics.TxSubmitRequestsTotal().WithLabelValues(clientIP, ...) matches reliably
and removes fragile coupling to httptest defaults.
In `@internal/api/api.go`:
- Around line 391-394: The check redundantly tests txRejectErr for nil after
using errors.As; remove the dead-code part so the condition reads just `if
isRejected { ... }` (or directly use the boolean result of errors.As) when
handling CBOR responses; update the branch that currently reads `if isRejected
&& txRejectErr != nil` to `if isRejected` and keep using txRejectErr inside the
block (symbols: txRejectErr, errors.As, r.Header.Get("Accept")).
- Around line 445-486: The realClientIP helper currently reparses trustedProxies
per request and naively returns the leftmost X-Forwarded-For value; fix it by
pre-parsing trustedProxies at startup into a structure (e.g., []net.IPNet and
[]net.IP) and change realClientIP to accept that pre-parsed list instead of
[]string so it avoids net.ParseCIDR/net.ParseIP on each request, then implement
XFF parsing right-to-left: split X-Forwarded-For, iterate from last to first,
skip entries whose IPs are in the trusted set and return the first untrusted hop
(falling back to X-Real-IP if appropriate and finally peerHost), and ensure you
stop reusing the outer err variable (use new locals or var declarations) when
calling net.ParseCIDR/net.ParseIP.
In `@internal/metrics/metrics.go`:
- Around line 114-124: Remove the redundant nil-guards from the IncTxSubmitCount
and IncTxSubmitFailCount helpers so behavior matches RecordTxRequest and
RecordTxContent (which assume collectors are initialized in init());
specifically, eliminate the if <collector> != nil checks in the IncTxSubmitCount
and IncTxSubmitFailCount functions (references: IncTxSubmitCount,
IncTxSubmitFailCount, txSubmitRequestsTotal, txSubmitRequestsFailedTotal) and
call .Inc() unguarded like the other record helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe8687c6-959c-439e-b01a-64484bf4350f
📒 Files selected for processing (8)
go.modinternal/api/api.gointernal/api/api_test.gointernal/config/config.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gosubmit/txinfo.gosubmit/txinfo_test.go
✅ Files skipped from review due to trivial changes (3)
- go.mod
- submit/txinfo_test.go
- internal/metrics/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- submit/txinfo.go
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
from
ae7126c to
64b4493
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
submit/txinfo_test.go (2)
222-228: Minor: prefert.Fatal(or check result is nil) in the negative case.If
ParseTxInfoever regresses to return a non-nil*TxInfoalongside a nil error, the current assertion still passes silently. Usingt.Fatalkeeps the intent explicit and makes it obvious if a future change introduces a dereference below.Proposed tweak
-func TestParseTxInfo_InvalidCBOR(t *testing.T) { - t.Parallel() - _, err := ParseTxInfo([]byte("not-valid-cbor")) - if err == nil { - t.Error("expected error for invalid CBOR, got nil") - } -} +func TestParseTxInfo_InvalidCBOR(t *testing.T) { + t.Parallel() + info, err := ParseTxInfo([]byte("not-valid-cbor")) + if err == nil { + t.Fatal("expected error for invalid CBOR, got nil") + } + if info != nil { + t.Errorf("expected nil TxInfo on error, got %+v", info) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@submit/txinfo_test.go` around lines 222 - 228, TestParseTxInfo_InvalidCBOR should fail fast and assert both that error is non-nil and that the returned *TxInfo is nil; update the test to use t.Fatal when err is nil (or explicitly assert tx == nil) so a regression returning a non-nil *TxInfo with a nil error cannot pass silently—modify the TestParseTxInfo_InvalidCBOR invocation of ParseTxInfo and its assertions accordingly (check the variables from ParseTxInfo and call t.Fatal on unexpected nil error or unexpected non-nil tx).
65-73: Optional: consider exercising post-Alonzo output format.
buildMinimalConwayBodyuses legacy (pre-Babbage) map-encoded outputs (map[uint]any{0: addr, 1: amount}). Conway nodes still accept this form, andParseTxInfoonly inspects witness/body keys relevant to the signals under test, so this is fine for exercisingScriptType,HasMinting,HasReferenceInputs. If you later want to cover inline-datum / reference-script outputs as part of DeFi content signals, the post-Alonzo array form ([addr, value, datum_option, script_ref]) would be a more representative fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@submit/txinfo_test.go` around lines 65 - 73, buildMinimalConwayBody currently constructs outputs using the legacy map-encoded form (map[uint]any{0: addr, 1: amount}); to exercise post-Alonzo output format change the outputs entry in buildMinimalConwayBody to use the array form [addr, value, datum_option, script_ref] (e.g. a slice with address, uint64 amount, nil datum option, nil script ref) so tests can cover inline-datum/reference-script cases and more closely match post-Alonzo/Conway fixtures inspected by ParseTxInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@submit/txinfo_test.go`:
- Around line 222-228: TestParseTxInfo_InvalidCBOR should fail fast and assert
both that error is non-nil and that the returned *TxInfo is nil; update the test
to use t.Fatal when err is nil (or explicitly assert tx == nil) so a regression
returning a non-nil *TxInfo with a nil error cannot pass silently—modify the
TestParseTxInfo_InvalidCBOR invocation of ParseTxInfo and its assertions
accordingly (check the variables from ParseTxInfo and call t.Fatal on unexpected
nil error or unexpected non-nil tx).
- Around line 65-73: buildMinimalConwayBody currently constructs outputs using
the legacy map-encoded form (map[uint]any{0: addr, 1: amount}); to exercise
post-Alonzo output format change the outputs entry in buildMinimalConwayBody to
use the array form [addr, value, datum_option, script_ref] (e.g. a slice with
address, uint64 amount, nil datum option, nil script ref) so tests can cover
inline-datum/reference-script cases and more closely match post-Alonzo/Conway
fixtures inspected by ParseTxInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c0f1a9f-166c-4337-975c-8ca020403614
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modinternal/api/api.gointernal/api/api_test.gointernal/config/config.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gosubmit/txinfo.gosubmit/txinfo_test.go
✅ Files skipped from review due to trivial changes (3)
- submit/txinfo.go
- internal/metrics/metrics_test.go
- internal/config/config.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/api/api_test.go
- internal/api/api.go
- internal/metrics/metrics.go
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
2 times, most recently
from
ee1ca5f to
9eba703
Compare
verbotenj
changed the title
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
7 times, most recently
from
3ad28a8 to
fc4e0a2
Compare
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
worktree-feat-improve-metrics
branch
from
fc4e0a2 to
0fe5faa
Compare
2 participants
Closes #32
Summary by cubic
Adds Prometheus counters for request outcomes and DeFi-focused transaction content in the
tx-submitAPI, and adds trusted‑proxy client IP handling for per‑IP logs. Keeps legacy gauges and centralizes metrics registration.New Features
tx_submit_requests_total{result}withaccepted/rejected/error. Skips increment on invalid Content‑Type; recordserroron body read/CBOR/submit failures.tx_submit_script_type_total{type},tx_submit_has_minting_total{has_minting},tx_submit_has_reference_inputs_total{has_reference_inputs}viasubmit.ParseTxInfo; recorded on CBOR parse success (accept and reject).api.trustedProxies/API_TRUSTED_PROXIES(IP or CIDR). UsesX-Real-IP/X-Forwarded-Foronly when the peer is trusted; otherwise falls back toRemoteAddr.Refactors
internal/metricswithRegister()/RegisterForTesting();Start()callsmetrics.Register(). Addedsubmit/txinfo.gofor content parsing and updatedinternal/apito use it.Written for commit 0fe5faa. Summary will update on new commits.
Summary by CodeRabbit
New Features
Configuration
Tests
Chores