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

4.x: Move archetype metrics under GreetService #7612

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

tvallin
Copy link
Member

@tvallin tvallin commented Sep 18, 2023

Description

fix #7611

Removed the MetricsService and moved the metrics logic to GreetService

Documentation

no impact

@tvallin tvallin self-assigned this Sep 18, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 18, 2023
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

I see that the generated test assertions now check only for the presence of the expected metrics and not the values. It would be nice if there was a way to verify the value as well, as we did before. I know it can be tedious to get the output, parse the output and save the current value, then access an endpoint, then retrieve the output and parse it again, and finally make sure the change in value is correct (rather than the absolute value) but would that work?

@tvallin
Copy link
Member Author

tvallin commented Sep 19, 2023

Yes that would work. Other possibility is to use ordered test method and run metric test first so we can avoid parsing metrics endpoint output for SE. Would you like this approach ?

Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I am reluctant to approve the PR because there will be more changes to how the archetype should generate the SE code once Tomas finishes some current work.

Also, ideally we would avoid depending on test ordering among the test methods. If Romain is OK with it then I will be as well.

@tvallin
Copy link
Member Author

tvallin commented Sep 21, 2023

That's okay, I just removed the ordered test and parse the metrics instead to get the values.

@tvallin tvallin force-pushed the refactor-archetype branch 2 times, most recently from 40ac2c9 to 33c7dd6 Compare September 26, 2023 12:29
@tjquinno
Copy link
Member

  1. You have probably already seen the conflict now in observerability.xml.

  2. I cloned your repo and switched to your refactor-archetype and rebuilt Helidon and the archetypes.

    When I run helidon init using the local cli-data the dialog for a custom SE project prompts for a metrics provider (Microprofile or Micrometer). If the user asks for metrics in the custom SE dialog, the pom should not ask further metrics-related questions (similar to the recent change to the 3.x archetype).

    A generated SE project with metrics support should include these metrics-related dependencies:

         <dependency>
             <groupId>io.helidon.webserver.observe</groupId>
             <artifactId>helidon-webserver-observe-metrics</artifactId>
         </dependency>
         <dependency>
             <groupId>io.helidon.metrics</groupId>
             <artifactId>helidon-metrics-system-meters</artifactId>
             <scope>runtime</scope>
         </dependency>

    This might be a change from when we first talked about what metrics dependencies should be in 4.x SE projects. For now, at least, we have decided not to use the helidon-metrics bundle in our SE metrics examples.

Signed-off-by: tvallin <thibault.vallin@oracle.com>
Signed-off-by: tvallin <thibault.vallin@oracle.com>
Signed-off-by: tvallin <thibault.vallin@oracle.com>
Signed-off-by: tvallin <thibault.vallin@oracle.com>
Signed-off-by: tvallin <thibault.vallin@oracle.com>
@barchetta barchetta merged commit d693ccb into helidon-io:main Sep 27, 2023
12 checks passed
@tvallin tvallin deleted the refactor-archetype branch September 27, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x archetypes metrics OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.x QuickStart SE archetype creates unneeded and confusingly-named MetricsService
3 participants