Skip to content

Update ResourceNotificationServiceLogging#4693

Merged
DamianEdwards merged 3 commits intomainfrom
damianedwards/resource-notification-service-logs
Jun 27, 2024
Merged

Update ResourceNotificationServiceLogging#4693
DamianEdwards merged 3 commits intomainfrom
damianedwards/resource-notification-service-logs

Conversation

@DamianEdwards
Copy link
Copy Markdown
Member

@DamianEdwards DamianEdwards commented Jun 27, 2024

Fixes #4686

Microsoft Reviewers: Open in CodeFlow

- Only log state text changes at debug level when the state text actually changes
- Log all published updates at the trace level
@DamianEdwards DamianEdwards requested review from davidfowl and removed request for mitchdenny June 27, 2024 01:27
@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 27, 2024
@mitchdenny
Copy link
Copy Markdown
Member

Is there any chance that we'll get two resource notifications for a resource being processed in parallel. I'm wondering if there is potential for resource states to rapidly oscillate and then we don't fire them.

@DamianEdwards
Copy link
Copy Markdown
Member Author

Is there any chance that we'll get two resource notifications for a resource being processed in parallel. I'm wondering if there is potential for resource states to rapidly oscillate and then we don't fire them.

The publish method takes a lock on the state, so all updates for a resource get effectively sequentialized anyway, so while races might result in "extra" transitions being logged, I'm not sure any can be missed, e.g.

  1. no state -> state1
  2. state2 update comes in
  3. state1 update comes in
  4. state2 and state1 updates race such that state1 gets the lock first
  5. no update is logged for state1 update as resource is already in that state
  6. state2 update is logged as transition from state1 to state2

If in step 4 state2 update gets the lock first, then state1 to state2 transition is logged, and then state1 update logs transition from state2 to state1 (the "extra" update).

If we wanted to solve that we could use a Channel or similar but ultimately calls to Publish still race each other until the sync point.

Comment thread tests/Aspire.Hosting.Tests/ResourceNotificationTests.cs
@DamianEdwards DamianEdwards merged commit 2680b78 into main Jun 27, 2024
@DamianEdwards DamianEdwards deleted the damianedwards/resource-notification-service-logs branch June 27, 2024 17:22
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ResourceNotificationService logs that a resource changed its state when it didn't

3 participants