You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/04/19 17:19:13 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2639: Add debug page on Monitor
milleruntime opened a new pull request, #2639:
URL: https://github.com/apache/accumulo/pull/2639
* Create a debug page on the monitor and a debug endpoint to display all
of the REST endpoints available on the monitor
* Also create a variable to store the live port number when the server
starts up
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1108565919
> I think there may be a way to get these dynamically from the Application Resources or by peeking at the class path and looking at the @GET and @POST annotations.
There may be a way to do this dynamically, but I don't know how and that seems more complicated.
> But, do we really want users to write applications to the Monitor REST API?
If we don't want users writing applications to get data from the Monitor, than that is fine. I wasn't trying to advertise any "implied reliability". The Monitor is very complex and we currently don't have any active web developers in the community to maintain it. I was only trying to provide developers and admins a way to see the data that is present through REST APIs that are present.
By having a link to the current REST endpoints I had originally called everything "debugging" for this reason. This page is nice for debugging what end points exist and what data is returned. The only way to do that now is to grep the code and try to assemble a proper URL from the strings. Then it is more work to try and get a valid URL from those strings... good luck. Here is the commands I ran on my development box to just obtain the rest endpoints.
`grep "rest" server/monitor/target/**/functions.js | sed -e 's/return $.getJSON(//g' | sed -e 's/var call = //g' | sed -e 's/, function(data) {//g'`
Then from those strings, you have to expunge the parameterized endpoints.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1104199829
> Isn't everything on the Monitor subject to change from version to version?
Yes, but we do try to keep the XML/JSON compatible and at a well-known location, because it can be used by automation.
> > I'm not sure who this listing of endpoints is expected to help, because the site is already pretty intuitive to navigate.
>
> Anyone? The one endpoint is broken and I don't even think anyone knew it was there. I added this to help with debugging.
If it's only for debugging, then should this be off by default?
The main concern I had here is "Implied reliability":
Advertisements of information generally come with the implication that people can rely on the information in the advertisement. As an analogy, a flyer with a notice that an item is on sale at a store comes with the implication that a consumer can rely on getting the sale price when they visit the store to purchase the item. Similarly, a telephone/address directory listening comes with the implication that a person/business can be contacted at that telephone/address. -- This new page feels to me like it's a kind of advertisement of information, in that it seems to imply to me that those pages will exist and contain the expected information. Conversely, this implication does not seem present to me in the normal user experience of navigating menus on the site because our menus and other hyperlinks are more navigational, whereas this is more of an advertisement of information. In other words, with our current links, the information can be expected to be found only because the link is
clickable... not because the information's address has been published in a listening, as is the case with this new page.
Secondarily, I'm also concerned about addition of new navigation on the site that doesn't substantially add information for users. It's an extra link with extraneous information. Extraneous information, and extra paths to the same information, can increase confusion and decrease usability. To maximize user experience and minimize potential confusion, we need to balance between convenience and conciseness. Imagine having a button to take you to some piece of information. Now, imagine adding a second button "for convenience" that takes you to the same information. Perhaps this second button displays the information very slightly differently (FWIW, Vanguard.com's website does this right now, and it's very frustrating), or perhaps it takes you to the exact same information. In either case, it can be confusing to users. They may ask "what's the difference between these two?" or "do I need to visit both links to avoid missing out on important information?" or "these look the same, but
are they actually the same?". Even the most well-intentioned changes for convenience can make a site slightly more confusing overall, even if it is an improvement for the use case it was intended.
> > I'd be concerned about users relying on these endpoints as fixed. They have the JSON/XML endpoints to rely on for information that the monitor knows... they should not rely on these endpoints for any kind of automation, permanent bookmarks, etc., since they are subject to change.
>
> They are fixed. It isn't easy to change them without breaking the monitor. I would argue that we should treat the endpoints better so users can rely on them for stuff like that.
They aren't fixed. We've already changed them a few times during 2.1's development. We renamed master to manager (granted that was a special case), we changed a numeric type from integer to long, we added external compaction stuff, we added an endpoint to append log events and renamed the one to clear logs, we separated the garbage collection information into a separate endpoint and removed the old endpoints, and possibly breaking formatting changes to several of the String-based values as well.
While it's probably a good idea to have fixed endpoints at some point, it's difficult to do this when these need drive the monitor. They will need to change as our needs for the monitor change. Their primary task is to empower the views, not to provide automation to users. That's what the dedicated JSON/XML endpoints are for.
These were also not designed to be consumed by end users. If they had been, I think we should have spent more time on deciding what these should look like... we probably wouldn't have 10 of 23 endpoints dedicated to separate `/rest/statistics/time` values, and then also had a `/rest/tservers/serverStats`, but then aggregated everything about the manager into a single `/rest/manager` endpoint. These are what you get when you are trying to solve an immediate problem of getting data to the page... they are not what you get when you design them for end-user automated consumption.
I'm not opposed to having fixed rest endpoints at some point, well documented and ready for automated consumption by users... but I don't think what we have now are anywhere close to what those should look like. Maybe with a bit of design effort, they could be, but not right now.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1103859770
@EdColeman I renamed the endpoint to "all".
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1104454940
> @ctubbsii do you want me to revert this change? Is there any follow on work you would like done?
Personally, I think it should be reverted (except the changes in logging the port that was used should be kept... that was a good change). Reverting is the position I've been arguing for above. However, I'm open to other positions, if a strong case can be made for doing something else.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1103768564
> Would catalog, inventory or even help be a more fit name than debug? Debug seems to imply something different than a list of end points.
Maybe. I was thinking we could dump more stuff on the end point then just all the REST APIs.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1102978853
Would catalog, inventory or even help be a more fit name than debug? Debug seems to imply something different than a list of end points.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1111139437
> > I think we should just put it at /rest without any menu linking to it
>
> I had tried this by using an annotation in `WebViews` but it didn't seem to work. I wonder if its cause we set the Servlet in Monitor here: ` server.addServlet(getRestServlet(), "/rest/*");`
I briefly looked into locating it at `/rest`... it's probably more trouble than it's worth to put it there.
Also, I don't see the problem with just relying on the WADL, that's provided by default with Jersey applications. There is a missing runtime dependency to get it working for the rest/ endpoints, though. I've fixed that in #2654
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2639:
URL: https://github.com/apache/accumulo/pull/2639
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#discussion_r854142311
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java:
##########
@@ -446,4 +446,49 @@ public Map<String,Object> getReplication() {
return model;
}
+
+ @GET
+ @Path("all")
+ @Template(name = "/default.ftl")
+ public Map<String,Object> getRestView() {
+
+ Map<String,Object> model = getModel();
+ model.put("title", "Rest Endpoints");
+
+ model.put("template", "debug.ftl");
+ model.put("js", "functions.js");
+
+ model.put("endpoints", getEndpoints());
+
+ return model;
+ }
+
+ private List<String> getEndpoints() {
+ List<String> endpoints = new ArrayList<>();
+ endpoints.add("/rest/manager");
+ endpoints.add("/rest/tables/namespaces");
+ endpoints.add("/rest/problems/summary");
+ endpoints.add("/rest/tables");
+ endpoints.add("/rest/tservers");
+ endpoints.add("/rest/scans");
+ endpoints.add("/rest/bulkImports");
+ endpoints.add("/rest/tservers/serverStats");
+ endpoints.add("/rest/tservers/recovery");
+ endpoints.add("/rest/logs");
+ endpoints.add("/rest/problems/details");
+ endpoints.add("/rest/replication");
+ endpoints.add("/rest/statistics/time/ingestRate");
+ endpoints.add("/rest/statistics/time/scanEntries");
+ endpoints.add("/rest/statistics/time/ingestByteRate");
+ endpoints.add("/rest/statistics/time/queryByteRate");
+ endpoints.add("/rest/statistics/time/load");
+ endpoints.add("/rest/statistics/time/lookups");
+ endpoints.add("/rest/statistics/time/minorCompactions");
+ endpoints.add("/rest/statistics/time/majorCompactions");
+ endpoints.add("/rest/statistics/time/indexCacheHitRate");
+ endpoints.add("/rest/statistics/time/dataCacheHitRate");
+ endpoints.add("/rest/status");
Review Comment:
Producing this statically means that this is almost certainly not going to be kept up-to-date. I think there may be a way to get these dynamically from the Application Resources or by peeking at the class path and looking at the `@GET` and `@POST` annotations.
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java:
##########
@@ -446,4 +446,49 @@ public Map<String,Object> getReplication() {
return model;
}
+
+ @GET
+ @Path("all")
+ @Template(name = "/default.ftl")
+ public Map<String,Object> getRestView() {
+
+ Map<String,Object> model = getModel();
+ model.put("title", "Rest Endpoints");
+
+ model.put("template", "debug.ftl");
+ model.put("js", "functions.js");
+
+ model.put("endpoints", getEndpoints());
+
+ return model;
+ }
+
+ private List<String> getEndpoints() {
+ List<String> endpoints = new ArrayList<>();
Review Comment:
This should be a sorted set.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1103891167
I am not sure why its failing on JDK 17. But I can look into that as a follow on.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1108901139
If the point is to help with debugging rest endpoints, and we don't want to advertise to users (and I don't think we should), then I think we should just put it at /rest without any menu linking to it. The alternative is to make it off by default, and have a flag that turns it on. However, I prefer just putting it at /rest and removing the menu option. As a follow-on, I would be willing to investigate trying to get it to be auto-generated from the webapp resources.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1103956045
> I'm not convinced it's a good idea to add this without a disclaimer at the top of the template that these pages are subject to change from version to version.
Isn't everything on the Monitor subject to change from version to version?
> I'm not sure who this listing of endpoints is expected to help, because the site is already pretty intuitive to navigate.
Anyone? The one endpoint is broken and I don't even think anyone knew it was there. I added this to help with debugging.
> I'd be concerned about users relying on these endpoints as fixed. They have the JSON/XML endpoints to rely on for information that the monitor knows... they should not rely on these endpoints for any kind of automation, permanent bookmarks, etc., since they are subject to change.
They are fixed. It isn't easy to change them without breaking the monitor. I would argue that we should treat the endpoints better so users can rely on them for stuff like that.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1109505078
> I think we should just put it at /rest without any menu linking to it
I had tried this by using an annotation in `WebViews` but it didn't seem to work. I wonder if its cause we set the Servlet in Monitor here: ` server.addServlet(getRestServlet(), "/rest/*");`
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#discussion_r855441877
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java:
##########
@@ -446,4 +446,49 @@ public Map<String,Object> getReplication() {
return model;
}
+
+ @GET
+ @Path("all")
+ @Template(name = "/default.ftl")
+ public Map<String,Object> getRestView() {
+
+ Map<String,Object> model = getModel();
+ model.put("title", "Rest Endpoints");
+
+ model.put("template", "debug.ftl");
+ model.put("js", "functions.js");
+
+ model.put("endpoints", getEndpoints());
+
+ return model;
+ }
+
+ private List<String> getEndpoints() {
+ List<String> endpoints = new ArrayList<>();
+ endpoints.add("/rest/manager");
+ endpoints.add("/rest/tables/namespaces");
+ endpoints.add("/rest/problems/summary");
+ endpoints.add("/rest/tables");
+ endpoints.add("/rest/tservers");
+ endpoints.add("/rest/scans");
+ endpoints.add("/rest/bulkImports");
+ endpoints.add("/rest/tservers/serverStats");
+ endpoints.add("/rest/tservers/recovery");
+ endpoints.add("/rest/logs");
+ endpoints.add("/rest/problems/details");
+ endpoints.add("/rest/replication");
+ endpoints.add("/rest/statistics/time/ingestRate");
+ endpoints.add("/rest/statistics/time/scanEntries");
+ endpoints.add("/rest/statistics/time/ingestByteRate");
+ endpoints.add("/rest/statistics/time/queryByteRate");
+ endpoints.add("/rest/statistics/time/load");
+ endpoints.add("/rest/statistics/time/lookups");
+ endpoints.add("/rest/statistics/time/minorCompactions");
+ endpoints.add("/rest/statistics/time/majorCompactions");
+ endpoints.add("/rest/statistics/time/indexCacheHitRate");
+ endpoints.add("/rest/statistics/time/dataCacheHitRate");
+ endpoints.add("/rest/status");
Review Comment:
I believe typical state of the art for REST endpoint documentation is [Swagger](https://swagger.io/). But, do we really want users to write applications to the Monitor REST API?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2639: Add debug page on Monitor
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2639:
URL: https://github.com/apache/accumulo/pull/2639#issuecomment-1104427371
@ctubbsii do you want me to revert this change? Is there any follow on work you would like done?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org