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

Setting default-web-module disables /metrics - FISH-785 #4924

Open
thehpi opened this issue Sep 30, 2020 · 8 comments
Open

Setting default-web-module disables /metrics - FISH-785 #4924

thehpi opened this issue Sep 30, 2020 · 8 comments
Assignees
Labels
Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev Type: Bug Label issue as a bug defect

Comments

@thehpi
Copy link

thehpi commented Sep 30, 2020

Description


When I set the default-web-module, e.g using asadmin:

set configs.config.server-config.http-service.virtual-server.server.default-web-module=test-web

Then when I request the root context / this will be forwarded to test-web application so these calls do the same

  • /test-web/rest/hello/world
  • /rest/hello/world

However the /metrics endpoint doesn't work anymore after this setting. This kind of makes sense because the root context is mapped to my webapp. But it doesn't feel ok that the /metrics endpoint (and probably more like /health) don't work anymore.

Expected Outcome

I would expect the /metrics endpoint to keep on working and return things like

# HELP vendor_system_cpu_load Display the "recent cpu usage" for the whole system. This value is a double in the [0.0,1.0] interval. A value of 0.0 means that all CPUs were idle during the recent period of time observed, while a value of 1.0 means that all CPUs were actively running 100% of the time during the recent period being observed. All values betweens 0.0 and 1.0 are possible depending of the activities going on in the system. If the system recent cpu usage is not available, the method returns a negative value.
vendor_system_cpu_load 0.02313624678663239
# TYPE base_memory_maxHeap_bytes gauge
# HELP base_memory_maxHeap_bytes Displays the maximum amount of memory in bytes that can be used for HeapMemory.
base_memory_maxHeap_bytes 7.29808896E9
# TYPE base_memory_committedNonHeap_bytes gauge

Current Outcome

When the default web module is set then the /metrics call returns 404.

I did find the following in MetricsServletContainerInitializer

public class MetricsServletContainerInitializer implements ServletContainerInitializer {

@Override
public void onStartup(Set<Class<?>> set, ServletContext ctx) throws ServletException {

    MetricsServiceConfiguration configuration = Globals.getDefaultBaseServiceLocator()
            .getService(MetricsServiceConfiguration.class);
    if (!"".equals(ctx.getContextPath())) {
        return;
    }

This indicates that metrics is only enabled for the root context. But this is only called on deployment AND when clearing the default-web-module. When setting the default web module this is not called but the /metrics endpoint doesn't work afterwards. So it looks like this relates to the problem but is not the only one.

Steps to reproduce (Only for bug reports)

See: https://github.com/thehpi/metrics-with-default-web-module

Follow the README

Environment

  • Payara Version: tested with 5.201 and 5.2020.4
  • Edition: Full
  • JDK Version: 8 Zulu
  • Operating System: Mac
@OndroMih
Copy link
Contributor

Yes, the MetricsServletContainerInitializer only binds to the root context if the application is already bound to the root context. If it becomes the default web module after it's deployed, it's not reinitialized and /metrics isn't accessible.

The current code of MetricsServletContainerInitializer works for Payara Micro, which can deploy an application to the root context at boot, but doesn't work with Payara Server, if the app is first deployed under an application context and then it becomes the default module and serves the root context without redeployment.

@OndroMih
Copy link
Contributor

OndroMih commented Oct 5, 2020

Hi @jGauravGupta, in case @thehpi would like to create a PR with a fix, can you help him how to know whether the application is the default webmodule?

@OndroMih
Copy link
Contributor

OndroMih commented Oct 5, 2020

I assume that all what's needed is to add a hook to the code that sets an app as the default web module and install the MetricsServlet, similarly to how it's installed during deployment in case the context root is empty. And also another hook to clean up if an app is unset as the default module.

@fturizo fturizo added Status: Open Issue has been triaged by the front-line engineers and is being worked on verification Type: Bug Label issue as a bug defect labels Nov 11, 2020
@MeroRai MeroRai assigned MeroRai and unassigned OndroMih Nov 20, 2020
@MeroRai
Copy link
Member

MeroRai commented Nov 24, 2020

Hi @thehpi,

Thank you you for providing us with a very detailed reproducer. You seem to be very familiar with our codebase, is this something you will be interested in creating a PR with a fix?

@MeroRai MeroRai added Status: Pending Waiting on the issue requester to give more details or share a reproducer and removed Status: Open Issue has been triaged by the front-line engineers and is being worked on verification labels Nov 24, 2020
@jGauravGupta
Copy link
Contributor

Reproduced the issue and created the internal ticket FISH-785 to address it.

@MeroRai MeroRai changed the title Setting default-web-module disables /metrics Setting default-web-module disables /metrics FISH-785 Nov 25, 2020
@MeroRai MeroRai added Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev and removed Status: Pending Waiting on the issue requester to give more details or share a reproducer labels Nov 25, 2020
@MeroRai MeroRai changed the title Setting default-web-module disables /metrics FISH-785 Setting default-web-module disables /metrics - FISH-785 Nov 25, 2020
@thehpi
Copy link
Author

thehpi commented Nov 27, 2020

@MeroRai I'm not at all familiar with the payara codebase but I am familiar with using it :)
But I can investigate of course and find a solution. But I would need some pointers.

As OndroMih noted in an earlier comment MetricsServletContainerInitializer doesn't work in payara server if an app is made default module after deployment.

What I find strange is that it IS called when the default module is cleared.

I guess what we need is:

  • when the default module is set this initializer is also called (need a pointer where that is done)
  • the initializer should also initialize metrcs when the app is the default module. I need a pointer on how te determine what default module is. Should I access the server config (how). Or is there a higher level service I can call (which one)

If you can help me a bit I will be able to create a PR

@jGauravGupta
Copy link
Contributor

I created a commit for ref which fixes the issue for metrics endpoint and requires restart on config change: jGauravGupta@f8e09ad
A similar fix is needed for other MP endpoints

@thehpi
Copy link
Author

thehpi commented Nov 30, 2020

I investigated why the setting default-web-module setting did not activate immediately but clearing it did.
I ended in com.sun.enterprise.web.WebContainer and found that when setting the default-web-module the current default web module (__default-web-module) is unloaded and the web module to become default is updated but not restarted, hence the MetricsServiceContainerInitializer is not called. But when clearing the default-web-module the old default web module is actually (re)loaded and as such calls the initializer.
So the solution could be to also reload the web module that is configured to become the default web module, but I don't know if thats a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev Type: Bug Label issue as a bug defect
Projects
None yet
Development

No branches or pull requests

5 participants