chore: Log fusion cache in memory entry #1527
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Check: build / build-and-test
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
[failure] 94-94:
Merge conflict marker encountered
[failure] 105-105:
Merge conflict marker encountered
[failure] 109-109:
Merge conflict marker encountered
[failure] 94-94:
Merge conflict marker encountered
[failure] 105-105:
Merge conflict marker encountered
[failure] 109-109:
Merge conflict marker encountered
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
2-2: 🛠️ Refactor suggestion
Remove unnecessary using directive
If the reflection-based approach is removed as suggested above, this using directive becomes unnecessary.
-using System.Reflection;Likely invalid or redundant comment.
| <<<<<<< HEAD | ||
| var mcaField = typeof(FusionCache).GetField("_mca", BindingFlags.NonPublic | BindingFlags.Instance); | ||
| var mcaValue = mcaField?.GetValue(_partiesCache); | ||
| var mcField = mcaValue!.GetType().GetField("_cache", BindingFlags.NonPublic | BindingFlags.Instance); | ||
| var mcValue = mcField?.GetValue(mcaValue) as IMemoryCache; | ||
|
|
||
| var inMemoryCacheValue = mcValue!.TryGetValue(cacheKey, out var inMemoryCacheEntry); | ||
| var inMemoryCacheEntryValue = inMemoryCacheEntry?.GetType().GetProperty("Value")?.GetValue(inMemoryCacheEntry); | ||
|
|
||
| _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@inMemoryCacheEntryValue}", | ||
| cacheKey, inMemoryCacheValue, inMemoryCacheEntryValue); | ||
| ======= | ||
| var inMemoryCacheValue = _inMemoryCache.TryGetValue<AuthorizedPartiesResult>(cacheKey, out var inMemoryCacheEntry); | ||
| _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@InMemoryCacheEntry}", | ||
| cacheKey, inMemoryCacheValue, inMemoryCacheEntry); | ||
| >>>>>>> main |
There was a problem hiding this comment.
The current implementation has several issues:
- There's an unresolved merge conflict between two different approaches to cache logging
- The HEAD version uses reflection to access private fields (
_mca,_cache), which is problematic because:- It breaks encapsulation
- It's fragile and may break with FusionCache updates
- It has performance overhead
- It makes the code harder to maintain
Consider these alternatives:
- Use the existing
IMemoryCacheapproach from the main branch (simpler and safer) - If additional cache diagnostics are needed, consider:
- Using FusionCache's public API for diagnostics
- Adding proper logging/metrics through FusionCache's events/callbacks
- Contacting FusionCache maintainers to request a proper diagnostics API
Here's the recommended approach (using main's version):
- var mcaField = typeof(FusionCache).GetField("_mca", BindingFlags.NonPublic | BindingFlags.Instance);
- var mcaValue = mcaField?.GetValue(_partiesCache);
- var mcField = mcaValue!.GetType().GetField("_cache", BindingFlags.NonPublic | BindingFlags.Instance);
- var mcValue = mcField?.GetValue(mcaValue) as IMemoryCache;
-
- var inMemoryCacheValue = mcValue!.TryGetValue(cacheKey, out var inMemoryCacheEntry);
- var inMemoryCacheEntryValue = inMemoryCacheEntry?.GetType().GetProperty("Value")?.GetValue(inMemoryCacheEntry);
-
- _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@inMemoryCacheEntryValue}",
- cacheKey, inMemoryCacheValue, inMemoryCacheEntryValue);
+ var inMemoryCacheValue = _inMemoryCache.TryGetValue<AuthorizedPartiesResult>(cacheKey, out var inMemoryCacheEntry);
+ _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@InMemoryCacheEntry}",
+ cacheKey, inMemoryCacheValue, inMemoryCacheEntry);📝 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.
| <<<<<<< HEAD | |
| var mcaField = typeof(FusionCache).GetField("_mca", BindingFlags.NonPublic | BindingFlags.Instance); | |
| var mcaValue = mcaField?.GetValue(_partiesCache); | |
| var mcField = mcaValue!.GetType().GetField("_cache", BindingFlags.NonPublic | BindingFlags.Instance); | |
| var mcValue = mcField?.GetValue(mcaValue) as IMemoryCache; | |
| var inMemoryCacheValue = mcValue!.TryGetValue(cacheKey, out var inMemoryCacheEntry); | |
| var inMemoryCacheEntryValue = inMemoryCacheEntry?.GetType().GetProperty("Value")?.GetValue(inMemoryCacheEntry); | |
| _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@inMemoryCacheEntryValue}", | |
| cacheKey, inMemoryCacheValue, inMemoryCacheEntryValue); | |
| ======= | |
| var inMemoryCacheValue = _inMemoryCache.TryGetValue<AuthorizedPartiesResult>(cacheKey, out var inMemoryCacheEntry); | |
| _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@InMemoryCacheEntry}", | |
| cacheKey, inMemoryCacheValue, inMemoryCacheEntry); | |
| >>>>>>> main | |
| var inMemoryCacheValue = _inMemoryCache.TryGetValue<AuthorizedPartiesResult>(cacheKey, out var inMemoryCacheEntry); | |
| _logger.LogInformation("In memory cache value for {CacheKey}, success: {InMemoryCacheValue} value: {@InMemoryCacheEntry}", | |
| cacheKey, inMemoryCacheValue, inMemoryCacheEntry); |
🧰 Tools
🪛 GitHub Check: build / build-and-test
[failure] 94-94:
Merge conflict marker encountered
[failure] 105-105:
Merge conflict marker encountered
[failure] 109-109:
Merge conflict marker encountered
[failure] 94-94:
Merge conflict marker encountered
[failure] 105-105:
Merge conflict marker encountered
[failure] 109-109:
Merge conflict marker encountered
|
Summary by CodeRabbit
New Features
Bug Fixes