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

[WFLY-17678] Add Micrometer quickstart #662

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Conversation

jasondlee
Copy link
Contributor

https://issues.redhat.com/browse/WFLY-17678

Add module
Develop demo app
Add README

@jasondlee jasondlee marked this pull request as ready for review June 5, 2023 17:07
@jasondlee jasondlee changed the title [WFLY-17679] Add Micrometer quickstart [WFLY-17678] Add Micrometer quickstart Jul 10, 2023
@jasondlee jasondlee force-pushed the WFLY-17678 branch 2 times, most recently from e9e44ec to 79e9952 Compare September 29, 2023 00:37
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Some changes needed, some changes suggested and there are a couple resources missing:

  1. the Helm Chart at charts/helm.yaml (see https://docs.google.com/document/d/1GFnZrnlwRkwC0HfziRws2UUWR99KdaQayA7Ei12SGN4/edit#heading=h.sldbnukpvc2 )
  2. the CI workflow at .github/workflows/quickstart_microprofile-micrometer_ci.yml ( see https://docs.google.com/document/d/1GFnZrnlwRkwC0HfziRws2UUWR99KdaQayA7Ei12SGN4/edit#heading=h.c4iuyi3bm84h )

micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved
micrometer/pom.xml Show resolved Hide resolved
micrometer/pom.xml Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved
micrometer/pom.xml Show resolved Hide resolved
@emmartins
Copy link
Contributor

Following the README instructions it's failing to deploy on current WFLY main (dist/target/wildfly-30.0.0.Final-SNAPSHOT/bin/standalone.sh), perhaps it needs another config? If so you should specify which in the README.adoc attributes at top

micrometer/pom.xml Outdated Show resolved Hide resolved
@emmartins
Copy link
Contributor

Following the README instructions it's failing to deploy on current WFLY main (dist/target/wildfly-30.0.0.Final-SNAPSHOT/bin/standalone.sh), perhaps it needs another config? If so you should specify which in the README.adoc attributes at top

Ok it seems this is just a README issue, later in the doc it's required to reconfig the server, I will pinpoint in sources...

micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Outdated Show resolved Hide resolved
@jasondlee
Copy link
Contributor Author

@emmartins Changes made and pushed.

@jasondlee jasondlee force-pushed the WFLY-17678 branch 2 times, most recently from 1cce209 to 1ddb325 Compare October 2, 2023 13:47
@jasondlee jasondlee requested a review from emmartins October 2, 2023 13:47
@kabir
Copy link
Contributor

kabir commented Oct 9, 2023

/retest

@jasondlee jasondlee force-pushed the WFLY-17678 branch 2 times, most recently from ef3c303 to bef8e05 Compare November 15, 2023 16:45
micrometer/pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Please rebase and do changes on comments.

.github/workflows/quickstart_micrometer_ci.yml Outdated Show resolved Hide resolved
micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Show resolved Hide resolved
micrometer/README.adoc Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved
shared-doc/build-the-quickstart-for-openshift.adoc Outdated Show resolved Hide resolved
shared-doc/build-the-quickstart-for-openshift.adoc Outdated Show resolved Hide resolved
@jasondlee jasondlee force-pushed the WFLY-17678 branch 2 times, most recently from 960d357 to 568672c Compare December 13, 2023 19:25
@jasondlee
Copy link
Contributor Author

@emmartins I believe I have addressed all of the requests here. Please verify and let me know if I missed anything. On to the next one... :)

@jasondlee jasondlee requested a review from emmartins December 13, 2023 19:25
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Everything looks good, but functionally the docker service is not working as expected with bootable-jar and provisioned-server, I believe this is due to not configuring the micrometer endpoint, which can be solved by packaging a cli script with the following command:

/subsystem=micrometer:write-attribute(name=endpoint, value="http://localhost:4318/v1/metrics")

Now thinking a bit more about this, I can imagine this kind of design will have trouble on OpenShift too, cause we need similar script packaged on openshift profile yet we don't actually know the metrics service hostname. Related, I guess we also need extra README content on how to setup the metrics service for OpenShift. Alternatively, we could say there that setup and usage of the metrics service is not OpenShift compatible, that only the app is compatible, but I think that would make us look as lazy.

@emmartins
Copy link
Contributor

emmartins commented Dec 14, 2023

Everything looks good, but functionally the docker service is not working as expected with bootable-jar and provisioned-server, I believe this is due to not configuring the micrometer endpoint, which can be solved by packaging a cli script with the following command:

/subsystem=micrometer:write-attribute(name=endpoint, value="http://localhost:4318/v1/metrics")

Now thinking a bit more about this, I can imagine this kind of design will have trouble on OpenShift too, cause we need similar script packaged on openshift profile yet we don't actually know the metrics service hostname. Related, I guess we also need extra README content on how to setup the metrics service for OpenShift. Alternatively, we could say there that setup and usage of the metrics service is not OpenShift compatible, that only the app is compatible, but I think that would make us look as lazy.

Wrt packaging a script to do such change, you can use same approach as the telemetry one, and change configure-micrometer.cli to be

# CLI script to enable micrometer for the quickstart application in the application server
if (outcome != success) of /extension=org.wildfly.extension.micrometer:read-resource
    /extension=org.wildfly.extension.micrometer:add()
    /subsystem=micrometer:add()
end-if

# Configure the endpoint for metrics publication
/subsystem=micrometer:write-attribute(name=endpoint, value="http://localhost:4318/v1/metrics")

and then you package it to the wiildfly and wildfly-jar plugin configs:

                            <!-- use cli script(s) to configure the server -->
                            <packaging-scripts>
                                <packaging-script>
                                    <scripts>
                                        <script>${basedir}/configure-micrometer.cli</script>
                                    </scripts>
                                </packaging-script>
                            </packaging-scripts>

This still has the OpenShift trouble of the localhost hostname usage in the script tho.

PS: please note the removal of reload command, such command atm has issues with provisioning so please remove it from all scripts (valid for telemetry script too)

@emmartins
Copy link
Contributor

provisioned-server and bootable-jar now works, thanks.

@emmartins
Copy link
Contributor

WFLY 31.0.0.Beta1 has been released, please rebase this PR with upstream/main branch, and update:

  • All pom.xml to 31.0.0.Final-SNAPSHOT
  • All pom.xml to 6
  • Property named version.server to 31.0.0.Beta1
  • Property named version.plugin.wildfly to 4.2.1.Final

micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/README.adoc Outdated Show resolved Hide resolved
micrometer/docker-compose.yaml Outdated Show resolved Hide resolved
Add module
Develop demo app
Document QS
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

LGTM now, @kstekovi you good to go on this one too

Copy link

openshift-ci bot commented Jan 18, 2024

@jasondlee: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/wildfly-quickstart-e2e a5d9418 link true /test wildfly-quickstart-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Tested baremetal and provisioning locally, LGTM.

@kstekovi
Copy link
Contributor

kstekovi commented Jan 22, 2024

everything LGTM. I don't have any topics for discussion.

The OpenShift part is not ready yet.

@emmartins emmartins merged commit 649dfd6 into wildfly:main Jan 22, 2024
19 of 20 checks passed
@emmartins
Copy link
Contributor

emmartins commented Jan 22, 2024

merged, thank you both, please don't forget we now should work on a new PR properly handling OpenShift

@jasondlee jasondlee deleted the WFLY-17678 branch January 22, 2024 20:08
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