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

Add Microprofile Health #25186

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

Thihup
Copy link
Contributor

@Thihup Thihup commented Oct 16, 2024

This change supports the deployment of MicroProfile Health 4.0 compatible applications, and modifies GF full profile to pass the MP Health 4.0.1 TCK by implementing the specification from scratch.

The MicroProfile Health uses the MicroProfile Config to check for two properties, so instead of requiring an explicit dependency,it handles the absence of the MP Config by returning a default value.

Currently this implementation does not add any default procedure.
Each application now contains four new endpoints: /health, /health/live, /health/ready and /health/started.

The TCK requires a single property (mp.health.disable-default-procedures) to be set (although currently unused by the implementation)

Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
@arjantijms
Copy link
Contributor

Wow, super cool! Just curious, how many of the TCK tests are passing at the moment?

Maybe the manifest.mf file can be generated? It usually is, but I didn't check yet if you're doing anything special. Just a quick glance.

@Thihup
Copy link
Contributor Author

Thihup commented Oct 17, 2024

Tests passed/Failed/Skipped: | 27/1/0
The only test failing is the testEmptyReadinessWithConfig.

The main issue related to this failure is use of the Microprofile Config. As I'm creating a new MpConfigProviderResolver, it is not reading the microprofile-config.properties.

As I understand, the correct use should be ConfigProvider.getConfig(), but using it fails with No ConfigProviderResolver implementation found!. I couldn't understand why it was happening as I required the implementation (Import-Package: io.helidon.config.mp;).

Related to the manifest, I had to change it to allow the use of a higher CDI version. I followed the pattern of the microprofile-jwt-auth-api.

@Thihup
Copy link
Contributor Author

Thihup commented Oct 17, 2024

Another issue: right now I'm registering the Servlet as the following http://localhost:8080/my-app/health, but the spec requires it to be http://localhost:8080/my-app/health.

@OndroMih
Copy link
Contributor

OndroMih commented Oct 17, 2024

@Thihup, regarding "As I understand, the correct use should be ConfigProvider.getConfig(), but using it fails with No ConfigProviderResolver implementation found!", this is probably related to classloading or the order in which MP Health and Config are initialized. Config in GF is initialized in https://github.com/eclipse-ee4j/glassfish/blob/master/appserver/microprofile/config/src/main/java/org/glassfish/microprofile/config/ConfigDeployer.java. Maybe it happens after MP Health is initialized and thus Config is not yet available. If it's the case, we need to ensure that Health initializes after Config.

Alternatively, refactor ConfigDeployer.java to extract the initialization into a separate method that initializes MP Config if it's not initialized, and call the method directly from the MP Health initializer or via a CDI event triggered from the Health initializer. I prepared something like that in my local branch with experiments related to MP Config, which I only have locally and haven't prepared as a PR yet:

@Override
    public ApplicationContainer load(Container container, DeploymentContext deploymentContext) {
        initializeConfigProviders();
        return new ConfigApplicationContainer(deploymentContext);
    }

    /**
     * Initialise Config providers if they haven't been initialized
     */
    public void initializeConfigProviders() {
        if (!configProvidersInitialized) {
            final var resolver = new MpConfigProviderResolver();
            ConfigProviderResolver.setInstance(resolver);
            configProvidersInitialized = true;
        }
    }

And then ConfigDeployer could be injected into the Health initializer and call the initializeConfigProviders() method from there to ensure Config is initialized. Or, even better, fire an HK2 event from Health initializer and handle the event by calling the method.

@arjantijms
Copy link
Contributor

Related to the manifest, I had to change it to allow the use of a higher CDI version. I followed the pattern of the microprofile-jwt-auth-api.

You are absolutely right. I missed it being in a patches folder.

(maybe for the future we can add some extension somewhere in OSGi so that we can more directly specify we allow higher versions, without patching the entire manifest)

Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
@Thihup Thihup marked this pull request as ready for review October 29, 2024 18:54
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
Signed-off-by: Thiago Henrique Hüpner <[email protected]>
@dmatej dmatej added this to the 7.1.0 milestone Oct 31, 2024
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.

4 participants