Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HybridCache stability and logging improvements #5467

Merged
merged 30 commits into from
Nov 4, 2024
Merged

HybridCache stability and logging improvements #5467

merged 30 commits into from
Nov 4, 2024

Conversation

mgravell
Copy link
Member

@mgravell mgravell commented Oct 3, 2024 •
edited
Loading

Stability (i.e. when bad things happen, it should be as gentle as possible) and logging:

  • implement ILogger logging of significant events
  • add basic EventSource capture
  • enforce MaximumPayloadBytes, fixes HybridCache MaximumPayloadBytes not operating per documentation #5432
  • ensure that instability of the underlying cache backend is not exposed to the caller
  • handle [de]serialization failures appropriately
  • enforce MaximumKeyLength (do not use cache for invalid keys)
Microsoft Reviewers: Open in CodeFlow

@joperezr joperezr changed the base branch from dev to main October 3, 2024 17:31
@joperezr
Copy link
Member

joperezr commented Oct 3, 2024

Hey @mgravell, as FYI I have re-targeted this PR to main branch as that is now our 9.0 branch going forward. Nothing should change from your point of view.

@RussKie RussKie requested a review from a team October 9, 2024 04:55
mgravell and others added 6 commits October 22, 2024 11:03
@mgravell
- handle serialization failures
a874d1c 
- enforce payload quota
- enforce key validity
- add proper logging (infrastructure failure: needs attn)

# Conflicts:
#	src/Libraries/Microsoft.Extensions.Caching.Hybrid/Microsoft.Extensions.Caching.Hybrid.csproj
@mgravell
- add "callback" to .dic
3951c72 
- log deserialization failures
- expose serialization failures
- tests for serialization logging scenarios
@mgravell
support and tests for stability despite unreliable L2
aac82e8
@mgravell
nit
6fd3047
@joperezr @mgravell
Compile for NS2.0
8a7b59b
@mgravell
include enabled check in our log output
5605ce5
@mgravell mgravell marked this pull request as ready for review October 22, 2024 12:33
@mgravell mgravell requested a review from a team as a code owner October 22, 2024 12:33
amcasey
amcasey reviewed Oct 25, 2024
View reviewed changes
BrennanConroy
BrennanConroy reviewed Oct 25, 2024
View reviewed changes
BrennanConroy
BrennanConroy approved these changes Oct 31, 2024
View reviewed changes
src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.StampedeStateT.cs
}
}
}
catch (Exception ex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to catch the (OperationCanceledException ex) when (SharedToken.IsCancellationRequested) ? (or ex.CancellationToken == SharedToken)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You resolved the conversation without making a change or comment. Would like to know the resolution here.

@mgravell
Copy link
Member Author

Pretty sure the CI failure is timing; will tweak tomorrow

@mgravell mgravell merged commit 9eea77d into dotnet:main Nov 4, 2024
6 checks passed
@mgravell mgravell mentioned this pull request Nov 5, 2024
Merged
joperezr added a commit to joperezr/extensions that referenced this pull request Nov 5, 2024
@mgravell @joperezr
HybridCache stability and logging improvements (dotnet#5467)
80c9263 
* - handle serialization failures
- enforce payload quota
- enforce key validity
- add proper logging (infrastructure failure: needs attn)

# Conflicts:
#	src/Libraries/Microsoft.Extensions.Caching.Hybrid/Microsoft.Extensions.Caching.Hybrid.csproj

* - add "callback" to .dic
- log deserialization failures
- expose serialization failures
- tests for serialization logging scenarios

* support and tests for stability despite unreliable L2

* nit

* Compile for NS2.0

* include enabled check in our log output

* add event-source tracing and counters

* explicitly specify event-source guid

* satisfy the stylebot overloads

* nix SDT

* fix failing CI test

* limit to net462

* PR feedback (all except event tests)

* naming

* add event source tests

* fix redundant comment

* add clarification

* more clarifications

* dance for our robot overlords

* drop Microsoft.Extensions.Telemetry.Abstractions package-ref

* fix glitchy L2 test

* better tracking for invalid event-source state

* reserve non-printable characters from keys, to prevent L2 abuse

* improve test output for ETW

* tyop

* ETW tests: allow longer if needed

* whitespace

* more ETW fixins

---------

Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
@joperezr joperezr mentioned this pull request Nov 5, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@amcasey amcasey Awaiting requested review from amcasey

Assignees

@mgravell mgravell

Labels
area-caching-hybrid
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HybridCache MaximumPayloadBytes not operating per documentation
5 participants
@mgravell @joperezr @BrennanConroy @amcasey @RussKie