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

Provide new plugin config to customize applicationMonitor configuration #1710

Open
scottkurz opened this issue Aug 3, 2023 · 5 comments · May be fixed by #1711
Open

Provide new plugin config to customize applicationMonitor configuration #1710

scottkurz opened this issue Aug 3, 2023 · 5 comments · May be fixed by #1711

Comments

@scottkurz
Copy link
Member

scottkurz commented Aug 3, 2023

BACKGROUND

(Opening this issue based on what started as an internal, IBM-only discussion here: https://github.ibm.com/liberty-dev-ex/design-issues/issues/16)

As described in this Liberty Tools Eclipse issue, a large app might take so long to restart that a user may want to disable the default "polled" application monitor behavior, and rely on debugger HCR to quickly test the results of the change.

PROPOSAL

A proposed design would be to have a new top-level config (e.g Maven parm/property), say 'appMonitorTrigger', and do do the following:

  • If set to one of the values, 'polled', 'mbean', 'disabled', LMP/LGP generates configDropin/overrides/liberty-plugin-app-monitor-config.xml in with (i.e. where the set value is 'polled', 'mbean', 'disabled').
  • If not set, LMP/LGP deletes configDropin/overrides/liberty-plugin-app-monitor-config.xml

The generation/deletion would happen at basically the same time (same goals/tasks) that the liberty-plugin-variable-config.xml file is generated today.

The Liberty Tools IDE features can then set this parm accordingly as they launch dev mode, depending on whether a Run or Debug is performed in Liberty Tools.

EXAMPLE

mvn liberty:dev -DappMonitorTrigger=disabled

will generate config:

 <applicationMonitor updateTrigger="disabled"/>

TODO

I believe this will merge with existing config but should double-check

Minor design questions:

  • Do we even need 'polled' as an option, since it's already the default? Is there really a reason to override any other config?
  • Do we want an explicit "not set" option in addition to defaulting? Seems like this could be helpful
@scottkurz
Copy link
Member Author

Note that we describe this as requiring a new plugin config, but the intended user of this support, Liberty Tools, would probably only ever do so through a property. So it's possibly worth raising the question: would we provide a behavior that was ONLY configurable via property, and NOT via plugin parameter?

@evie-lau
Copy link
Member

evie-lau commented Aug 7, 2023

I published a snapshot to Sonatype to test the described behavior using 3.8.3-appMon-SNAPSHOT. Currently can specify with: mvn liberty:dev -DapplicationMonitor=mbean. (I noticed your suggestion for appMonitorTrigger after publishing, but can change to this if preferred)
Has validation to ensure the value is one of: [polled, mbean, disabled]. If not specified, the existing file in configDropins/overrides is deleted.

Relevant PRs:

Snapshot Installation:

<plugin>
    <groupId>io.openliberty.tools</groupId>
    <artifactId>liberty-maven-plugin</artifactId>
    <version>3.8.3-appMon-SNAPSHOT</version>
</plugin>

And make sure to add this under the pom.xml <project>

<pluginRepositories>
    <!-- Configure Sonatype OSS Maven snapshots repository -->
    <pluginRepository>
        <id>sonatype-nexus-snapshots</id>
        <name>Sonatype Nexus Snapshots</name>
        <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
        <snapshots>
            <enabled>true</enabled>
        </snapshots>
        <releases>
            <enabled>false</enabled>
        </releases>
    </pluginRepository>
</pluginRepositories>

@scottkurz
Copy link
Member Author

In reviewing this with the broader runtime team, the question was raised: how does WDT/LDT handle an update to a web.xml in debug mode?

The reason for focusing on this case is that, as a non-Java change, it would fall outside the scope of the debugger's HCR capabilities to update or replace.

As it turns out, doing a test and looking at the source code: it appears that WDT/LDT will actually, in general, still call the mbean to update an app after a change, even with running in debug mode.

The handling of Java class file updates is actually a special case in the overall logic, as one can see here:
https://github.com/OpenLiberty/open-liberty-tools/blob/e86be3dbc36b3dfa531804f480babdfdafbd9638/dev/com.ibm.ws.st.jee.core/src/com/ibm/ws/st/jee/core/internal/JEEServerExtension.java#L525-L528
where the publish is NOT performed if the project change is a .class file AND debug mode is enabled.

@scottkurz
Copy link
Member Author

scottkurz commented Sep 25, 2023

Given the last comment, the new, proposed config becomes less immediately useful to Liberty Tools (IDE) function.

If we assume that the WDT/LDT-like mbean invocation would be added to Liberty Tools IDEs in the near-term, then this proposal would indeed seem useful. That mbean invocation function could include logic to update or not based on the type of resource (as in LDT/WDT).

However, without this mbean invocation function imminent, is it still worth going ahead with this proposal?

If we were to go ahead, we would probably make the debug behavior "opt-in", i.e. not the default, making it less useful. Would it be reasonable to doc for a user creating their own updateTrigger config variable and setting it (or not) in their Liberty Tools Run/Debug configs (i.e. using a custom config)?

I don't think there is a way to do this that doesn't involve some amt. of a learning curve for someone used to the WDT/LDT approach.

BTW, another thing WDT/LDT does is to implement org.eclipse.jdt.debug.core.IJavaHotCodeReplaceListener. I don't follow the logic completely, see here, but it seems to include being smart enough to offer a prompt to restart the server when the situation warrants.

@scottkurz
Copy link
Member Author

scottkurz commented Sep 27, 2023

**2023-09-27 - DXDI update: **

Decided to wait on merging this function until we have a more complete view of how we'd leverage this in Liberty Tools IDE, which would be a significant enhancement in each IDE, and which still requires some design work.

  • As discussed before, it'd be nice to have a doc showing how to workaround this problem in each IDE.
  • Suggested that when it comes time to prototyping app update detection and mbean invocation, it should be considered at the LMP/LGP layers (rather than assuming each LT IDE adds its own function).
    • Is there a way to move update detection (i.e. is it a Java change vs. web.xml vs. annotation change) into LMP/LGP?
  • Recognized that even the LDT/WDT app update detection might have some gaps, e.g. if a urlPatterns attr changes on a @WebServlet this would require an app update. Perhaps we'd even need to go further than the LDT/WDT function does today?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants