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.

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

fix: ensure proper application reload on configuration change and reg… #13

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

armineyvazi
Copy link
Collaborator

  1. Application Reload on Configuration Change:
    The application now properly reloads when a configuration change is detected. This fix improves the signal handling logic, ensuring that the application correctly reinitializes its environment when it receives a SIGHUP signal.

  2. Prometheus Metrics Registration:
    Introduced the sync.Once pattern to handle the registration of Prometheus metrics within the middleware. This ensures that the metrics are registered only once during the application's lifecycle, preventing any potential duplication or conflicts.

@armineyvazi armineyvazi requested review from Iajrdev and alipourhabibi and removed request for Iajrdev August 19, 2024 12:54
main.go Outdated
os.Exit(1)
}
}()

return server, nil
}

func configChanged(oldConfig *config.Config) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea for detecting the reload was that we only reload the services that their config was changed,
For example if only our log configs are changed we should not reload the echo server.
What do you think? @Iajrdev

Copy link
Collaborator

@Iajrdev Iajrdev left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Collaborator

@alipourhabibi alipourhabibi left a comment

Choose a reason for hiding this comment

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

LGMT

@Iajrdev Iajrdev merged commit 9aafac4 into PwPJ:main Aug 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants