-
-
Notifications
You must be signed in to change notification settings - Fork 506
Reduce noisy change events #2438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
5b319fb
f8449f0
15e27df
e3e1f6d
8895405
2e96f54
d9508d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,7 +117,8 @@ await _cache.RemoveAllAsync(new[] { | |
| }); | ||
|
|
||
| await _cache.SetAsync(GetTotalCacheKey(utcNow, organizationId), usage.Total, TimeSpan.FromHours(8)); | ||
| await _organizationRepository.SaveAsync(organization); | ||
| // Usage counters and last-event timestamps are operational updates, not user-facing entity changes. | ||
| await _organizationRepository.SaveAsync(organization, o => o.Notifications(false)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this usage save is the one that crosses the monthly or hourly limit, the backend can still emit Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in f8449f0: the Svelte websocket handler now recognizes PlanOverage and invalidates only the affected organization, organization stats, and organization list queries. The endpoint also evaluates pending usage so the refetch observes the transition. Covered by the targeted frontend query-invalidation test and OrganizationEndpointTests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an organization hits the hourly bucket limit, the new Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 15e27df: hourly throttle transitions now write a per-bucket marker before PlanOverage is published. Saving that completed bucket emits one OrganizationChanged while routine usage saves remain silent. Covered by SavePendingUsageAsync_HourlyThrottleClears_PublishesOrganizationChangedMessage; the full UsageServiceTests suite passes. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -204,7 +205,8 @@ await _cache.RemoveAllAsync(new[] { | |
|
|
||
| await _cache.SetAsync(GetTotalCacheKey(utcNow, project.OrganizationId, projectId), usage.Total, TimeSpan.FromHours(8)); | ||
|
|
||
| await _projectRepository.SaveAsync(project); | ||
| // Project configuration changes have their own save path and should remain the source of ProjectChanged messages. | ||
| await _projectRepository.SaveAsync(project, o => o.Notifications(false)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -257,8 +259,8 @@ public async Task HandleOrganizationChangeAsync(Organization modified, Organizat | |
|
|
||
| if (bucketTotal.Value >= bucketLimit) | ||
| { | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = modified.Id, IsHourly = true }); | ||
| await _cache.SetAsync(GetThrottledKey(utcNow, modified.Id), true, TimeSpan.FromMinutes(5)); | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = modified.Id, IsHourly = true }); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -449,8 +451,8 @@ public async Task IncrementTotalAsync(string organizationId, string projectId, i | |
| if (bucketTotal >= bucketLimit && bucketTotal - bucketLimit < eventCount) | ||
| { | ||
| // org will be throttled during the current bucket of time | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = organizationId, IsHourly = true }); | ||
| await _cache.SetAsync(GetThrottledKey(utcNow, organizationId), true, TimeSpan.FromMinutes(5)); | ||
| await _messagePublisher.PublishAsync(new PlanOverage { OrganizationId = organizationId, IsHourly = true }); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -929,6 +929,7 @@ private async Task AfterResultMapAsync<TDestination>(ICollection<TDestination> m | |
| currentUsage.Discarded = realTimeUsage.CurrentUsage.Discarded; | ||
| currentUsage.TooBig = realTimeUsage.CurrentUsage.TooBig; | ||
| currentUsage.Deleted = realTimeUsage.CurrentUsage.Deleted; | ||
| viewOrganization.IsOverMonthlyLimit = currentUsage.Limit >= 0 && currentUsage.Total >= currentUsage.Limit; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an organization's temporary event bonus expires without another usage bucket being saved, AGENTS.md reference: AGENTS.md:L67-L67 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 2aa10d3: real-time usage totals are now compared against the organization view model live effective monthly limit, so an expired bonus cannot keep the overage state false. The focused regression failed before the fix with Expected True, Actual False; OrganizationEndpointTests now pass 103 of 103 and the build is clean. |
||
|
|
||
| var currentHourUsage = viewOrganization.GetCurrentHourlyUsage(timeProvider); | ||
| currentHourUsage.Total = realTimeUsage.CurrentHourUsage.Total; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { QueryClient } from '@tanstack/svelte-query'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { invalidatePlanOverageQueries, queryKeys } from './api.svelte'; | ||
|
|
||
| describe('invalidatePlanOverageQueries', () => { | ||
| it('invalidates only the affected organization state', async () => { | ||
| // Arrange | ||
| const queryClient = new QueryClient(); | ||
| const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries').mockImplementation(async () => {}); | ||
|
|
||
| // Act | ||
| await invalidatePlanOverageQueries(queryClient, { | ||
| is_hourly: false, | ||
| organization_id: 'organization-id' | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(invalidateSpy).toHaveBeenCalledTimes(3); | ||
| expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: queryKeys.id('organization-id', undefined) }); | ||
| expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: queryKeys.id('organization-id', 'stats') }); | ||
| expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: queryKeys.list(undefined) }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,6 @@ | |
| import { ChangeType, type WebSocketMessageValue } from '$features/websockets/models'; | ||
| import { useEventListener } from 'runed'; | ||
| import { SvelteSet } from 'svelte/reactivity'; | ||
| import { debounce } from 'throttle-debounce'; | ||
|
|
||
| import FreePlanNotification from './notifications/free-plan-notification.svelte'; | ||
| import HourlyOverageNotification from './notifications/hourly-overage-notification.svelte'; | ||
|
|
@@ -95,19 +94,21 @@ | |
| ); | ||
| const requiresPremiumUpgrade = $derived(requiresPremium && !organization?.has_premium_features && !needsProjectConfiguration); | ||
|
|
||
| const refetchConfigurationState = debounce(1500, async () => { | ||
| await Promise.all([organizationQuery.refetch(), projectsQuery.refetch()]); | ||
| }); | ||
|
|
||
| useEventListener(document, 'PersistentEventChanged', (event) => { | ||
| const message = (event as CustomEvent<WebSocketMessageValue<'PersistentEventChanged'>>).detail; | ||
|
|
||
| if (message.change_type === ChangeType.Removed || message.organization_id !== currentOrganizationId.current || !message.project_id) { | ||
| const projectNeedsConfiguration = projects.some((project) => project.id === message.project_id && project.is_configured === false); | ||
|
|
||
| if ( | ||
| message.change_type === ChangeType.Removed || | ||
| message.organization_id !== currentOrganizationId.current || | ||
| !message.project_id || | ||
| !projectNeedsConfiguration || | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 910499a: valid PersistentEventChanged project IDs are now recorded immediately for the current organization, without requiring the projects query to have loaded first. Once project data arrives, the local configured-ID set suppresses the stale setup notification without refetching organization or project data. Added a focused early-event regression; Svelte validation is clean and all 404 frontend unit tests pass. |
||
| configuredProjectIds.has(message.project_id) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| configuredProjectIds.add(message.project_id); | ||
| void refetchConfigurationState(); | ||
| }); | ||
| </script> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { ChangeType } from '$features/websockets/models'; | ||
| import { render } from '@testing-library/svelte'; | ||
| import { tick } from 'svelte'; | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import OrganizationNotifications from './organization-notifications.svelte'; | ||
|
|
||
| const organizationRefetch = vi.hoisted(() => vi.fn()); | ||
| const projectsRefetch = vi.hoisted(() => vi.fn()); | ||
| const projects = vi.hoisted(() => [{ id: 'project-id', is_configured: false, organization_id: 'organization-id' }]); | ||
|
|
||
| vi.mock('$features/organizations/api.svelte', () => ({ | ||
| getOrganizationQuery: () => ({ data: undefined, refetch: organizationRefetch }), | ||
| getOrganizationsQuery: () => ({ data: { data: [] } }) | ||
| })); | ||
|
|
||
| vi.mock('$features/organizations/context.svelte', () => ({ | ||
| organization: { current: 'organization-id' } | ||
| })); | ||
|
|
||
| vi.mock('$features/projects/api.svelte', () => ({ | ||
| getOrganizationProjectsQuery: () => ({ data: { data: projects }, isSuccess: true, refetch: projectsRefetch }) | ||
| })); | ||
|
|
||
| vi.mock('$features/users/api.svelte', () => ({ | ||
| getMeQuery: () => ({ data: { organization_ids: ['organization-id'], roles: [] } }) | ||
| })); | ||
|
|
||
| describe('OrganizationNotifications', () => { | ||
| beforeEach(() => { | ||
| organizationRefetch.mockReset(); | ||
| projectsRefetch.mockReset(); | ||
| projects[0]!.is_configured = false; | ||
| }); | ||
|
|
||
| it('does not refetch organization or project state for persistent event changes', async () => { | ||
| render(OrganizationNotifications, { | ||
| isChatEnabled: false, | ||
| openChat: vi.fn() | ||
| }); | ||
|
|
||
| const message = { | ||
| change_type: ChangeType.Added, | ||
| organization_id: 'organization-id', | ||
| project_id: 'project-id' | ||
| }; | ||
|
|
||
| document.dispatchEvent(new CustomEvent('PersistentEventChanged', { detail: message })); | ||
| document.dispatchEvent(new CustomEvent('PersistentEventChanged', { detail: message })); | ||
| await tick(); | ||
|
|
||
| expect(organizationRefetch).not.toHaveBeenCalled(); | ||
| expect(projectsRefetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not refresh configuration state for an already configured project', async () => { | ||
| projects[0]!.is_configured = true; | ||
| render(OrganizationNotifications, { | ||
| isChatEnabled: false, | ||
| openChat: vi.fn() | ||
| }); | ||
|
|
||
| document.dispatchEvent( | ||
| new CustomEvent('PersistentEventChanged', { | ||
| detail: { | ||
| change_type: ChangeType.Added, | ||
| organization_id: 'organization-id', | ||
| project_id: 'project-id' | ||
| } | ||
| }) | ||
| ); | ||
| await tick(); | ||
|
|
||
| expect(organizationRefetch).not.toHaveBeenCalled(); | ||
| expect(projectsRefetch).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this repeatable migration repairs a current-month usage total from below the live limit to at or above it,
Notifications(false)removes theOrganizationChangedsignal that previously refreshed connected clients, while this path publishes no replacementPlanOverage. Users with warm Svelte or Angular organization caches therefore continue seeing the organization as under limit until an unrelated refresh; suppress routine migration notifications, but emit a targeted signal when the repaired current-month total crosses the effective limit.AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ee08a86: the repeatable usage repair migration now detects only a current-month transition from below the live effective monthly limit to at or above it, saves the repaired usage silently, then publishes one monthly PlanOverage. Repeat runs remain silent because the persisted total is already repaired. The regression reproduced the issue before the fix (Expected 1, Actual 0), runs the migration twice to prove one-shot behavior, and both UpdateEventUsageMigrationTests plus all 27 UsageServiceTests pass; the build is clean.