Automated Test: implement-device-limits #335
Conversation
* Anonymous: Add device limiter * break auth if limit reached * fix typo * refactored const to make it clearer with expiration * anon device limit for config --------- Co-authored-by: Eric Leijonmarck <eric.leijonmarck@gmail.com>
📝 WalkthroughWalkthroughConfiguration field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/services/anonymous/anonimpl/impl.go (1)
100-115:⚠️ Potential issue | 🟡 MinorBug: Wrong error variable logged.
Line 113 logs
err(the function parameter) instead oferrD(the delete error). This will lognilsince the function returns early on line 103 iferr != nil.🐛 Proposed fix
errD := a.anonStore.DeleteDevice(ctx, deviceID) if errD != nil { - a.log.Debug("Failed to untag device", "error", err) + a.log.Debug("Failed to untag device", "error", errD) }
🤖 Fix all issues with AI agents
In `@packages/grafana-runtime/src/config.ts`:
- Line 97: The property anonymousDeviceLimit is initialized to undefined without
a type, so add an explicit type annotation matching the GrafanaConfig interface
(e.g., anonymousDeviceLimit: number | undefined = undefined) where
anonymousDeviceLimit is declared/initialized to ensure the inferred type allows
merging numeric boot data and stays consistent with GrafanaConfig.
In `@pkg/services/anonymous/anonimpl/anonstore/database.go`:
- Around line 52-54: The wire binding references anonstore.ProvideAnonDBStore
but the constructor now requires a primitive deviceLimit (int64) that wire can't
inject; update the DI binding in pkg/server/wire.go to either remove the bare
reference to ProvideAnonDBStore and use manual initialization in impl.go, or
change the provider set to call ProvideAnonDBStore with a concrete deviceLimit
value (or a provider that returns int64) so the deviceLimit parameter is
satisfied; locate ProvideAnonDBStore and the wire binding entry for anonstore in
wire.go and ensure the provider set matches the new signature.
🧹 Nitpick comments (6)
pkg/services/anonymous/anonimpl/anonstore/database.go (2)
16-18: Export constant to avoid duplication.The
anonymousDeviceExpirationconstant is also defined inpkg/services/anonymous/anonimpl/api/api.go. Consider exporting this constant so it can be reused, ensuring consistency if the expiration window changes.Proposed fix
-const anonymousDeviceExpiration = 30 * 24 * time.Hour +// AnonymousDeviceExpiration defines the time window for tracking anonymous devices. +const AnonymousDeviceExpiration = 30 * 24 * time.HourThen in
api/api.go, import and useanonstore.AnonymousDeviceExpiration.
108-118: Document the soft limit behavior regarding concurrent requests.The check-then-act pattern between
CountDevicesand the insert/update is not atomic, allowing concurrent requests to potentially exceed the limit briefly. This is likely acceptable for a soft limit, but adding a code comment would clarify this intentional design decision.Suggested documentation
func (s *AnonDBStore) CreateOrUpdateDevice(ctx context.Context, device *Device) error { var query string - // if device limit is reached, only update devices + // If device limit is reached, only update existing devices. + // Note: This is a soft limit - concurrent requests may briefly exceed the limit + // due to the non-atomic check-then-act pattern. This is acceptable as the limit + // is enforced on subsequent requests. if s.deviceLimit > 0 {pkg/services/anonymous/anonimpl/api/api.go (1)
18-18: Consider importing the constant from anonstore.This constant duplicates
anonymousDeviceExpirationdefined inanonstore/database.go. If the exported constant suggestion from the database.go review is adopted, this file should import and useanonstore.AnonymousDeviceExpirationto maintain consistency.Proposed fix (after exporting from anonstore)
-const anonymousDeviceExpiration = 30 * 24 * time.HourThen update line 71:
- fromTime := time.Now().Add(-anonymousDeviceExpiration) + fromTime := time.Now().Add(-anonstore.AnonymousDeviceExpiration)pkg/services/anonymous/anonimpl/anonstore/database_test.go (1)
51-70: Good test coverage for the device limit error path.The test correctly validates that
ErrDeviceLimitReachedis returned when attempting to add a new device beyond the limit. However, consider adding a test case that validates theupdateDevicefallback path—when a device already exists and the limit is reached, an update to that existing device should succeed (per theCreateOrUpdateDevicelogic indatabase.go).💡 Suggested additional test case
func TestIntegrationDeviceLimitAllowsUpdates(t *testing.T) { store := db.InitTestDB(t) anonDBStore := ProvideAnonDBStore(store, 1) anonDevice := &Device{ DeviceID: "existing-device", ClientIP: "10.30.30.2", UserAgent: "test", UpdatedAt: time.Now().Add(-time.Hour), } err := anonDBStore.CreateOrUpdateDevice(context.Background(), anonDevice) require.NoError(t, err) // Update the same device - should succeed even at limit anonDevice.ClientIP = "10.30.30.3" anonDevice.UpdatedAt = time.Now() err = anonDBStore.CreateOrUpdateDevice(context.Background(), anonDevice) require.NoError(t, err) }pkg/services/anonymous/anonimpl/client.go (1)
30-59: Synchronous device tagging adds latency to authentication.The change from async to sync means the
Authenticatemethod now waits for the DB operation inTagDeviceto complete. While this enables proper error handling, it may increase authentication latency, especially under high load or slow DB conditions.If latency becomes a concern, consider whether the device limit check could be optimized (e.g., cached device count) or if non-critical tagging failures should still be handled asynchronously.
pkg/services/anonymous/anonimpl/impl.go (1)
144-148: Consider differentiating log messages for device limit vs other errors.The debug log "Failed to tag device for UI" treats all errors the same, but
ErrDeviceLimitReachedis expected behavior rather than a failure. Additionally, the caller inclient.go(line 49) logs a warning for non-limit errors, which could result in the same error being logged at both Debug and Warn levels.Consider either:
- Removing this log since the caller handles logging, or
- Differentiating the message for
ErrDeviceLimitReached💡 Option: Remove debug log to avoid double logging
err = a.tagDeviceUI(ctx, httpReq, taggedDevice) if err != nil { - a.log.Debug("Failed to tag device for UI", "error", err) return err }
| @@ -94,6 +94,7 @@ export class GrafanaBootConfig implements GrafanaConfig { | |||
| theme2: GrafanaTheme2; | |||
| featureToggles: FeatureToggles = {}; | |||
| anonymousEnabled = false; | |||
| anonymousDeviceLimit = undefined; | |||
There was a problem hiding this comment.
Add explicit type annotation for type consistency.
The property is initialized to undefined but lacks an explicit type annotation. TypeScript will infer the type as undefined rather than number | undefined, which could cause type errors when the actual numeric value is merged from boot data. This should match the interface definition in GrafanaConfig.
Proposed fix
- anonymousDeviceLimit = undefined;
+ anonymousDeviceLimit: number | undefined = undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| anonymousDeviceLimit = undefined; | |
| anonymousDeviceLimit: number | undefined = undefined; |
🤖 Prompt for AI Agents
In `@packages/grafana-runtime/src/config.ts` at line 97, The property
anonymousDeviceLimit is initialized to undefined without a type, so add an
explicit type annotation matching the GrafanaConfig interface (e.g.,
anonymousDeviceLimit: number | undefined = undefined) where anonymousDeviceLimit
is declared/initialized to ensure the inferred type allows merging numeric boot
data and stays consistent with GrafanaConfig.
| func ProvideAnonDBStore(sqlStore db.DB, deviceLimit int64) *AnonDBStore { | ||
| return &AnonDBStore{sqlStore: sqlStore, log: log.New("anonstore"), deviceLimit: deviceLimit} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if wire.go still references ProvideAnonDBStore and how it's used
# Find wire bindings for ProvideAnonDBStore
echo "=== Wire bindings for ProvideAnonDBStore ==="
rg -n "ProvideAnonDBStore" --type go
# Check if there's any direct usage in wire sets
echo -e "\n=== Full context of wire usage ==="
rg -n -B5 -A5 "anonstore\.ProvideAnonDBStore|ProvideAnonDBStore" --type go -g '*wire*'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1633
Remove or update wire binding for new constructor signature.
The wire binding at pkg/server/wire.go:375 references anonstore.ProvideAnonDBStore as a bare function pointer without providing the now-required deviceLimit parameter. Since deviceLimit is a primitive type (int64), wire cannot auto-inject it. This will cause a compilation error. Either remove the wire binding if manual initialization in impl.go is sufficient, or update it to provide the deviceLimit parameter.
🤖 Prompt for AI Agents
In `@pkg/services/anonymous/anonimpl/anonstore/database.go` around lines 52 - 54,
The wire binding references anonstore.ProvideAnonDBStore but the constructor now
requires a primitive deviceLimit (int64) that wire can't inject; update the DI
binding in pkg/server/wire.go to either remove the bare reference to
ProvideAnonDBStore and use manual initialization in impl.go, or change the
provider set to call ProvideAnonDBStore with a concrete deviceLimit value (or a
provider that returns int64) so the deviceLimit parameter is satisfied; locate
ProvideAnonDBStore and the wire binding entry for anonstore in wire.go and
ensure the provider set matches the new signature.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit