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/05/05 23:25:43 UTC
[GitHub] [accumulo] tchaie opened a new pull request, #2679: Add front-end console logging in Monitor
tchaie opened a new pull request, #2679:
URL: https://github.com/apache/accumulo/pull/2679
Closes #2659
---
Added console logs for REST calls that indicate:
- What was called
- JSON data string returned from call
REST calls used on the Monitor Overview page use `console.debug()` to avoid clogging up the console. These log messages are filtered out by default in some browsers but can be shown if toggled on. Otherwise, the rest of the REST calls use `console.info()`.
--
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] DomGarguilo commented on pull request #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#issuecomment-1119667586
These changes look good so far. It might be a good addition to add the type of REST call in the logged message. For example you could say "REST GET request to ..." or "REST POST request to ..."
--
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] DomGarguilo commented on a diff in pull request #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#discussion_r867035868
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -426,25 +473,34 @@ function getTraceOfType(type, minutes) {
*/
function getTraceShow(id) {
var call = '/rest/trace/show/' + id;
+ console.info("Call to " + call);
return $.getJSON(call, function(data) {
- sessionStorage.traceShow = JSON.stringify(data);
+ var jsonDataStr = JSON.stringify(data);
+ sessionStorage.traceShow = jsonDataStr;
+ console.info("REST call to " + call + " stored in sessionStorage.traceShow = " + jsonDataStr);
});
}
/**
* REST GET call for the logs, stores it on a sessionStorage variable
*/
function getLogs() {
- return $.getJSON('/rest/logs', function(data) {
- sessionStorage.logs = JSON.stringify(data);
+ var call = '/rest/logs';
+ console.info("Call to " + call);
+ return $.getJSON(call, function(data) {
+ var jsonDataStr = JSON.stringify(data);
+ sessionStorage.logs = jsonDataStr;
+ console.info("REST call to " + call + " stored in sessionStorage.logs = " + jsonDataStr);
});
Review Comment:
There are also setters and getters for the session storage variables that might make it easier when refactoring shared code into a function: https://www.w3schools.com/jsref/prop_win_sessionstorage.asp
--
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 #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#discussion_r867010362
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -426,25 +473,34 @@ function getTraceOfType(type, minutes) {
*/
function getTraceShow(id) {
var call = '/rest/trace/show/' + id;
+ console.info("Call to " + call);
return $.getJSON(call, function(data) {
- sessionStorage.traceShow = JSON.stringify(data);
+ var jsonDataStr = JSON.stringify(data);
+ sessionStorage.traceShow = jsonDataStr;
+ console.info("REST call to " + call + " stored in sessionStorage.traceShow = " + jsonDataStr);
});
}
/**
* REST GET call for the logs, stores it on a sessionStorage variable
*/
function getLogs() {
- return $.getJSON('/rest/logs', function(data) {
- sessionStorage.logs = JSON.stringify(data);
+ var call = '/rest/logs';
+ console.info("Call to " + call);
+ return $.getJSON(call, function(data) {
+ var jsonDataStr = JSON.stringify(data);
+ sessionStorage.logs = jsonDataStr;
+ console.info("REST call to " + call + " stored in sessionStorage.logs = " + jsonDataStr);
});
Review Comment:
A lot of these do the same thing. You can probably simplify these by placing them in a new function that takes a parameter for the session storage variable. In this case, it would be "logs". I found https://stackoverflow.com/a/2132814/196405 , which suggests sessionStorage.logs is equivalent to: sessionStorage["logs"], so you could pass "logs" as a parameter.
Also, I'm thinking these console messages should probably use `console.debug`, at least for the second one.
The "Call to " one could probably stay console.info. It might also be better worded as: "Retrieving " or "Loading data from " instead of "Call to ".
--
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] tchaie commented on pull request #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
tchaie commented on PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#issuecomment-1121173820
Will work on cleaning up commits.
--
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 #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#discussion_r870639422
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -299,19 +320,15 @@ function getNamespaceTables(namespaces) {
namespaceList = namespaces.toString();
var call = '/rest/tables/namespaces/' + namespaceList;
- return $.getJSON(call, function(data) {
- sessionStorage.tables = JSON.stringify(data);
- });
+ return getJSONForTable(call, 'tables');
Review Comment:
It probably doesn't really matter, but some of these that create the variable, and only use it once inside the function call, could just inline the variable to turn these into one-liners:
```javascript
return getJSONForTable('/rest/tables/namespaces/' + namespaceList, 'tables');
```
There are several such occurrences in this PR that could be changed like this.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -460,6 +456,7 @@ function clearTableProblems(tableID) {
call = sanitize(call);
// make the rest call, passing success function callback
$.post(call, function () {
+ console.info("REST POST call to " + call);
refreshProblems();
});
Review Comment:
I think this pattern shows up a couple of times and could be put into it's own function (the following is only pseudo-code, so I don't expect it to work as-is):
```javascript
function doLoggedPostCall(endpoint, callback) {
console.info("POST call to " + endpoint);
$.post(endpoint, function() {
console.debug("POST call to " + endpoint + " : success"); // not sure the appropriate message here
if (callback != null) {
// execute callback; for example, refreshProblems() in this case... could be something else in other cases.
}
}
}
```
Then, This becomes:
```javascript
doLoggedPostCall(call, refreshProblems);
```
--
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 #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2679:
URL: https://github.com/apache/accumulo/pull/2679#discussion_r868313863
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -256,24 +256,50 @@ function createTableCell(index, sortValue, showValue) {
'">' + showValue + '</td>';
}
+/**
+ * Builds console logging message for REST GET calls
+ * @param {string} REST url called
+ * @param {object} data returned from REST call
+ * @param {string} session storage/global variable to hold REST data
+ */
+ function createRestGetLog(call, data, sessionDataVar){
+ var jsonDataStr = JSON.stringify(data);
+
+ //Handle data to be stored in global variable instead of session storage
+ if (sessionDataVar==="NAMESPACES") {
+ NAMESPACES = jsonDataStr;
+ console.debug("REST GET call to " + call +
+ " stored in " + sessionDataVar + " = " + NAMESPACES);
+ }
+ else {
+ sessionStorage[sessionDataVar] = jsonDataStr;
+ console.debug("REST GET request to " + call +
+ " stored in sessionStorage." + sessionDataVar + " = " + sessionStorage[sessionDataVar]);
+ }
+ }
+
///// REST Calls /////////////
/**
* REST GET call for the manager information,
* stores it on a sessionStorage variable
*/
function getManager() {
- return $.getJSON('/rest/manager', function(data) {
- sessionStorage.manager = JSON.stringify(data);
+ var call = '/rest/manager';
+ console.info("Retrieving " + call);
+ return $.getJSON(call, function(data) {
+ createRestGetLog(call, data, 'manager');
});
}
Review Comment:
Since the same pattern is used repeatedly, this code could be simplified even further, by replacing it with a new function, something like `getJSONforTable('/rest/manager', 'manager');`
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js:
##########
@@ -507,20 +563,25 @@ function getProblemDetails() {
* stores it on a sessionStorage variable
*/
function getReplication() {
- return $.getJSON('/rest/replication', function(data) {
- sessionStorage.replication = JSON.stringify(data);
+ var call = '/rest/replication';
+ console.info("Retrieving " + call);
+ return $.getJSON(call, function(data) {
+ createRestGetLog(call, data, 'replication');
});
}
//// Overview Plots Rest Calls
+//// Note: console.debug messages may not show by default in some browsers
Review Comment:
I think the logging level concept is pretty standard, so you probably don't need this comment here.
```suggestion
```
Or, you could clarify that the choice of using debug was intentional:
```suggestion
//// Note: console.debug avoids spamming the console by default in browsers that support it
```
--
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 merged pull request #2679: Add front-end console logging in Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2679:
URL: https://github.com/apache/accumulo/pull/2679
--
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