Skip to content

Add health checks for Event Hub component#4050

Closed
oising wants to merge 8 commits intomicrosoft:release/8.0from
oising:eventhubs-healthcheck
Closed

Add health checks for Event Hub component#4050
oising wants to merge 8 commits intomicrosoft:release/8.0from
oising:eventhubs-healthcheck

Conversation

@oising
Copy link
Copy Markdown
Contributor

@oising oising commented May 1, 2024 •
edited by dotnet-policy-service Bot
Loading

Add healthchecks for eventhub component

closes #3980

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-codeflow for labeling automated codeflow. intentionally a different color! label May 1, 2024
@dotnet-policy-service dotnet-policy-service Bot added Servicing-consider Issue for next servicing release review community-contribution Indicates that the PR has been added by a community member labels May 1, 2024
@oising oising changed the title initial commit Add health checks for Event Hub component May 1, 2024
@oising oising marked this pull request as ready for review May 1, 2024 15:14
danmoseley
danmoseley reviewed May 2, 2024
View reviewed changes
Comment thread src/Components/Aspire.Azure.Messaging.EventHubs/EventHubsComponent.cs
danmoseley
danmoseley reviewed May 2, 2024
View reviewed changes
Comment thread src/Components/Aspire.Azure.Messaging.EventHubs/EventHubsComponent.cs
{
Mode = EventHubsRetryMode.Exponential,
MaximumRetries = 3,
Delay = TimeSpan.FromMilliseconds(800),
Copy link
Copy Markdown
Member

@danmoseley danmoseley May 2, 2024

Choose a reason for hiding this comment

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

where is 800 from?

Copy link
Copy Markdown
Contributor Author

@oising oising May 2, 2024

Choose a reason for hiding this comment

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

I'll hoist this into Aspire:Settings :)

Copy link
Copy Markdown
Contributor

@sebastienros sebastienros May 9, 2024

Choose a reason for hiding this comment

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

@eerhardt I don't think there is a precedent for health check retry options. I found MongoDBSettings.HealthCheckTimeout only.

Would adding a RetryOptions HealthCheckRetryOptions property in AzureMessagingEventHubsSettings work here?

@oising
Copy link
Copy Markdown
Contributor Author

oising commented May 2, 2024

image

If you look at the eventhubs playground API project (it's the only webapp based one capable of exposing /health) you can trace the logic.

I also added a DisableHealthChecks setting to align with other components.

@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 7, 2024

@oising thanks for the PR and for your contribution. I noticed that your PR is targeting 8.0 branch, which we are no longer taking changes for. Is there a specific reason why you picked release/8.0 as the target branch and would you be okay with us retargeting to main branch instead?

@oising
Copy link
Copy Markdown
Contributor Author

oising commented May 7, 2024

@oising thanks for the PR and for your contribution. I noticed that your PR is targeting 8.0 branch, which we are no longer taking changes for. Is there a specific reason why you picked release/8.0 as the target branch and would you be okay with us retargeting to main branch instead?

Retarget as you please!

@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 7, 2024 •
edited
Loading

Actually, I was about to retarget, but because this has changes that are only on release/8.0 branch that we specifically don't want in main, things wouldn't go so well. Due to that, I think the best way would be to reset and cherry-pick the changes directly into main's tip, and then update this PR. I can try helping assuming there are no merge conflicts. If there are, probably the easiest would be to close this and reopen a brand new one based on main.

@oising @sebastienros
Update src/Components/Aspire.Azure.Messaging.EventHubs/AzureMessaging…
b1ccd65 
…EventHubsSettings.cs

Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
@oising oising mentioned this pull request May 10, 2024
Merged
@oising
Copy link
Copy Markdown
Contributor Author

oising commented May 10, 2024

Created new PR targeting main (cherry pick) at #4139

/cc @joperezr @danmoseley @sebastienros

@oising oising closed this May 10, 2024
@oising oising deleted the eventhubs-healthcheck branch May 10, 2024 15:59
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sebastienros sebastienros sebastienros left review comments

+1 more reviewer

@danmoseley danmoseley danmoseley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-codeflow for labeling automated codeflow. intentionally a different color! community-contribution Indicates that the PR has been added by a community member Servicing-consider Issue for next servicing release review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@oising @joperezr @sebastienros @danmoseley