Add caching for integration template properties #6553
Conversation
|
Claude finished @brant-livefront's task —— View job Code Review CompleteSummaryThis PR adds local memory caching for integration template properties (Organization, Group, OrganizationUser lookups) to reduce database calls during event processing. The implementation addresses most previous review findings effectively. Previous Review StatusFixed:
Not Addressed:
New Findings💭 Finding 1: Null results bypass cache and trigger repeated database calls (IntegrationTemplatePropertyCache.cs:29) 🎨 Finding 3: Repetitive cache registration code could be extracted into helper method (ServiceCollectionExtensions.cs:945) Good Practices ObservedThread-safe concurrent request handling with AsyncLazy pattern and comprehensive unit tests covering race conditions. Action Items
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
.../AdminConsole/Services/Implementations/EventIntegrations/IntegrationTemplatePropertyCache.cs
Outdated
Show resolved
Hide resolved
.../AdminConsole/Services/Implementations/EventIntegrations/IntegrationTemplatePropertyCache.cs
Show resolved
Hide resolved
.../AdminConsole/Services/Implementations/EventIntegrations/OrganizationUserUserDetailsCache.cs
Show resolved
Hide resolved
.../AdminConsole/Services/Implementations/EventIntegrations/IntegrationTemplatePropertyCache.cs
Outdated
Show resolved
Hide resolved
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## brant/datadog-template-improvements #6553 +/- ##
=======================================================================
+ Coverage 52.77% 52.79% +0.01%
=======================================================================
Files 1910 1914 +4
Lines 84890 85001 +111
Branches 7582 7586 +4
=======================================================================
+ Hits 44800 44875 +75
- Misses 38354 38389 +35
- Partials 1736 1737 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
I dig it, but want some other eyes on this.
| @@ -0,0 +1,64 @@ | |||
| using System.Collections.Concurrent; | |||
There was a problem hiding this comment.
🎨 All this is something that I think is so generic and reusable that it could be in a more central / core location, for usage beyond event integrations.
| public AzureServiceBusSettings AzureServiceBus { get; set; } = new AzureServiceBusSettings(); | ||
| public RabbitMqSettings RabbitMq { get; set; } = new RabbitMqSettings(); | ||
| public int IntegrationCacheRefreshIntervalMinutes { get; set; } = 10; | ||
| public int UserCacheTtlMinutes { get; set; } = 60; |
There was a problem hiding this comment.
🎨 Per my other comment, if this is generic perhaps we also rethink how this is loaded and defined.
|
@JimmyVo16 per your recent work on the org cache, something like this albeit with a configurable Redis backing would meet the mark. |
…e-property-caching
| { | ||
| var loaded = await LoadValueFromRepositoryAsync(key); | ||
|
|
||
| if (loaded is not null) |
There was a problem hiding this comment.
💭 Finding 1: Null results bypass cache and trigger repeated database calls
The code only caches non-null values. If LoadValueFromRepositoryAsync returns null (e.g., entity not found), subsequent requests for the same key will repeatedly hit the database.
Impact: Performance degradation for lookups of non-existent entities, which could be common in event processing scenarios.
Options:
- Keep current behavior if null results are expected to be rare and should always be fresh
- Cache null results with a shorter TTL to prevent repeated DB calls for missing entities
- Document this intentional design decision
Is the current null-bypass behavior intentional for this use case?
There was a problem hiding this comment.
@withinfocus I had done this intentionally - none of the things that we are fetching should be null as they are actively referenced on an event. For instance, if the GroupId is set on an event, then that Group should exist. If it had been deleted recently or an edge case like that, my thinking was then we don't need to cache it anyway.
But if we are looking to make this more generic, I wonder if I should make it always cache nulls with a configurable short TTL.
| @@ -0,0 +1,8 @@ | |||
| using Bit.Core.Models.Data.Organizations.OrganizationUsers; | |||
|
|
|||
| namespace Bit.Core.Services; | |||
There was a problem hiding this comment.
This interface is in Bit.Core.Services while the other cache interfaces are in Bit.Core.AdminConsole.Services:
IGroupCache→Bit.Core.AdminConsole.ServicesIOrganizationCache→Bit.Core.AdminConsole.ServicesIOrganizationUserUserDetailsCache→Bit.Core.Services← inconsistent
Recommendation: Move this interface to Bit.Core.AdminConsole.Services to match the pattern and improve discoverability.
Previous review finding
This was identified as Finding 4 in the previous review but was not addressed in the recent commits.
There was a problem hiding this comment.
This has been fixed - I moved the other cache interfaces to Bit.Core.Services instead.
| services.TryAddSingleton<IIntegrationFilterService, IntegrationFilterService>(); | ||
| services.TryAddKeyedSingleton<IEventWriteService, RepositoryEventWriteService>("persistent"); | ||
|
|
||
| services.TryAddKeyedSingleton<IMemoryCache>( |
There was a problem hiding this comment.
🎨 Finding 3: Consider extracting cache registration into helper method
The three cache registrations (lines 945-980) follow identical patterns with only names/types differing. This repetition could be reduced.
Suggested refactoring
private static void RegisterPropertyCache<TInterface, TImplementation>(
this IServiceCollection services,
string cacheName,
int maxEntries,
int ttlMinutes,
Func<IServiceProvider, IMemoryCache, TimeSpan, TImplementation> factory)
where TImplementation : TInterface
where TInterface : class
{
services.TryAddKeyedSingleton<IMemoryCache>(
cacheName,
new MemoryCache(new MemoryCacheOptions { SizeLimit = maxEntries })
);
services.TryAddSingleton<TInterface>(provider =>
factory(
provider,
provider.GetRequiredKeyedService<IMemoryCache>(cacheName),
TimeSpan.FromMinutes(ttlMinutes)
)
);
}
// Usage:
services.RegisterPropertyCache<IOrganizationUserUserDetailsCache, OrganizationUserUserDetailsCache>(
"UserCache",
globalSettings.EventLogging.UserCacheMaxEntries,
globalSettings.EventLogging.UserCacheTtlMinutes,
(p, cache, ttl) => new OrganizationUserUserDetailsCache(
cache, ttl, p.GetRequiredService<IOrganizationUserRepository>())
);This aligns with @withinfocus's comment about making this more generic and reusable.
|
r-tome
left a comment
r-tome
left a comment
There was a problem hiding this comment.
Looks great to me! I would be grateful if you could just add some xmldoc to the new interfaces briefly explaining their purpose
|
Closing this in favor of the more robust version in #6608 |
📔 Objective
This PR adds 3 new local memory caches to reduce some of the calls to the repository for template parameters. For instance, when an event references a
GroupNamewe need to fetch the group form the database. With this local cache, that will live in a memory cache so frequently hitting the same properties won't require re-loading them.There are configurable amounts for the max size (number of items) and how many minutes the records should stay in the cache. The TTL is an absolute duration - i.e. if it's set to 30 and you change something, it will take at most 30 minutes to pickup the new change.
These are especially helpful in the Datadog template implementation which makes use of all of these properties and would most likely be used for all events for a given Organization.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes