ctubbsii commented on PR #5012: URL: https://github.com/apache/accumulo/pull/5012#issuecomment-2436221491
> The intention with this PR is to have two separate monitor (old and new) running off the same Monitor process until the new one is complete. All of this work could be done in a feature branch and _not_ merged into `main`. @DomGarguilo's needs something that serves up the data to create the new UI. I don't think it's necessary to do that. The pages can be served from the same server. There's no need to run two. Also, I didn't think it was in scope to create a whole new monitor from scratch... I thought the goal was to do a comprehensive review of what's there, get rid of the bits we want to drop, and add stuff we think should be there. That doesn't require running two servers. We can build off of the previous work, the previous design, etc. That's what I had been discussing with @DomGarguilo. If your goal is to just create a monitor to view published metrics, I can see that being a standalone server, separate from the existing monitor, but I don't think it would be a replacement. > I think it's simpler. With the current Monitor code you have to search for the class and method that has the matching path to understand which method is being called by the UI code. The way that the routes are built with Javalin, the mapping is centrally located in one place and can be made visible via the RouteOverview plugin to aid in debugging and development. I think it's pretty easy to look at the `@Path` annotations to see the mappings. We used to have a centralized view, with servlets, and it got better when we switched to this. I think it might only look better now because the number of endpoints without a UI is still very small. Once that starts growing, and you start adding all the features back in that we want to keep from the old monitor, that's going to get quite cumbersome. Preferring one over the other may still be subjective. But even if there are some small style benefits, I still strongly prefer building off what we have, rather than make the lateral move, and add all the new dependencies. > I think the intention here is to build the new Monitor off of exposed metric information. If there is something new that is needed, then we should add the metric for it. This approach means that the Monitor is not getting special data that can't be retrieved or calculated by just getting the metrics via some MeterRegistry. I don't think we should expose this via the public API in the AccumuloClient, I think that's the wrong approach. I think the fundamental difference in our perspective on this is that you seem to see the monitor as a way to get some metrics information about the system. And, I see it as a way to get an overview of the system's deployed state and current activity. Some of the deployed state and current activity can be viewed as metrics, but some of it simply can't be. There isn't a metric for instanceName or instanceId, for example, or list of tables, or list of resource groups, or list of zookeepers the system is configured with, or whether a table is currently "offline" or "disabled". These kinds of things are status things that don't make sense as metrics, but are instead retrieved from the public API. So, I absolutely think that the monitor does have a need of getting data that can't be retrieved or calculated by just getting the metrics via some MeterRegistry. To the extent that the monitor is showing stuff that is specifically metrics, or can be metrics, I think the monitor should do ver y little of that, and instead rely on the user's use of a dedicated metrics collection/aggregation/alerting/visualization thing. If that's the kind of thing you're trying to prototype here, I think that's fine, but perhaps it shouldn't have anything to do with the monitor at all? Or, to the extent that it is, it's a very specific page on the monitor, separate from the non-metrics stuff. So, I think we should discuss what the actual goal is here, because it currently feels like "metrics" is being used as a hammer to hit every nail, when a lot of the monitor isn't actually a nail... some of it is, and for that stuff, it's probably useful to prototype that like you're doing in this PR... but not all of it is. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
