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.

Sign up for GitHub

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

Jump to bottom

Enriching Hub Status to include Node info #6127

Merged
merged 2 commits into from
Jul 10, 2018
Merged

Enriching Hub Status to include Node info #6127

merged 2 commits into from
Jul 10, 2018

Conversation

krmahadevan
Copy link
Contributor

Currently the end-point /grid/api/hub/status
does not provide information about the nodes
attached to it and also the busy/free status
per browser flavor on each of the nodes.

Enriching the end-point to provide node info
when invoked with the option
/grid/api/hub/status?configuration=nodes

Sample payload as below:

{
  "nodes": [
    {
      "Id": "http://192.168.1.6:5555",
      "browsers": [
        {
          "browser": "safari",
          "slots": {
            "busy": 0,
            "total": 1
          }
        },
        {
          "browser": "chrome",
          "slots": {
            "busy": 0,
            "total": 5
          }
        },
        {
          "browser": "firefox",
          "slots": {
            "busy": 0,
            "total": 5
          }
        }
      ]
    }
  ],
  "success": true
}

@krmahadevan
Enriching Hub Status to include Node info
e527232 
Currently the end-point /grid/api/hub/status 
does not provide information about the nodes 
attached to it and also the busy/free status 
per browser flavor on each of the nodes.

Enriching the end-point to provide node info 
when invoked with the option 
/grid/api/hub/status?configuration=nodes

Sample payload as below:

{
  "nodes": [
    {
      "Id": "http://192.168.1.6:5555",
      "browsers": [
        {
          "browser": "safari",
          "slots": {
            "busy": 0,
            "total": 1
          }
        },
        {
          "browser": "chrome",
          "slots": {
            "busy": 0,
            "total": 5
          }
        },
        {
          "browser": "firefox",
          "slots": {
            "busy": 0,
            "total": 5
          }
        }
      ]
    }
  ],
  "success": true
}
@krmahadevan
Copy link
Contributor Author

ping @diemol @shs96c @barancev

@shs96c - As discussed in the SeConf in BLR, I have enriched the /grid/api/hub/status end-point to optionally provide node details when queried. I will send a separate PR for the node side.

@luke-hill
Copy link
Contributor

For my own clarity, would this provide essentially what http://url-to-grid/get_node_config?node=nameofnode does, but instead of being for 1 node it is for all nodes?

@krmahadevan
Copy link
Contributor Author

krmahadevan commented Jul 9, 2018 via email

@luke-hill
Copy link
Contributor

The top url I mentioned might be an SGE addon (Not sure).

Rightio. But sorry to be a pain. Isn't that provided (I hope this isn't SGE as well!), by url-to-grid/grid/console

@krmahadevan
Copy link
Contributor Author

krmahadevan commented Jul 9, 2018 via email •
edited
Loading

mach6
mach6 approved these changes Jul 9, 2018
View reviewed changes
Copy link
Member

@mach6 mach6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

My only minor suggestion -- "free" and "busy" could be statically declared strings.

@krmahadevan
Copy link
Contributor Author

@mach6 - Thanks for the comments. I have extracted out all repeating strings in that class and converted them into private constants. Please take a look.

@shs96c shs96c added the C-java label Jul 10, 2018
shs96c
shs96c reviewed Jul 10, 2018
View reviewed changes
Copy link
Member

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I was a man who asked for unicorns, I'd ask for a test of some description to ensure that this continues working....

I think we're nearly there. Thank you for all the work so far.

java/server/src/org/openqa/grid/web/servlet/HubStatusServlet.java Outdated
@@ -167,4 +185,57 @@ protected void process(
throw new IOException(e);
}
}

private static boolean IsKeyPresentIn(List<String> keys, String key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: java method names begin with a lower-case letter. We can fix this once the PR is merged in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. That should have been caught by me (I guess its the after effects of toggling between Java and Golang from work). Fixed it.

java/server/src/org/openqa/grid/web/servlet/HubStatusServlet.java Outdated
if (request.getParameter("configuration") != null && !"".equals(request.getParameter("configuration"))) {
keysToReturn = Arrays.asList(request.getParameter("configuration").split(","));
} else if (requestJSON != null && requestJSON.containsKey("configuration")) {
if (request.getParameter(CONFIGURATION) != null && !"".equals(request.getParameter(CONFIGURATION))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use !Strings.isNullOrEmpty(request.getParameter(CONFIGURATION)) here for a slightly clearer check here (I realise that this wasn't done before)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

java/server/src/org/openqa/grid/web/servlet/HubStatusServlet.java Outdated
keysToReturn = Arrays.asList(request.getParameter("configuration").split(","));
} else if (requestJSON != null && requestJSON.containsKey("configuration")) {
if (request.getParameter(CONFIGURATION) != null && !"".equals(request.getParameter(CONFIGURATION))) {
keysToReturn = Arrays.asList(request.getParameter(CONFIGURATION).split(","));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are empty values allowed in the list? If not Splitter.on(",").trimResults().omitEmptyStrings().splitToList() may be a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

java/server/src/org/openqa/grid/web/servlet/HubStatusServlet.java Outdated

private Map<String, Object> getNodeInfo(RemoteProxy remoteProxy) {
return ImmutableSortedMap.of(
"Id", remoteProxy.getId(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Id capitalised, but browser not? Maybe leave both lower case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

java/server/src/org/openqa/grid/web/servlet/HubStatusServlet.java
}

private static <T> Collector<T, ?, Integer> counting() {
return reducing(0, e -> 1, Integer::sum);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an IntStreams sum method, but presumably that isn't appropriate to use here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Collectors.counting() returns a long. I didn't want to make use of a long just to count a bunch of proxies. That is why I resorted to creating this reducing function

@krmahadevan krmahadevan force-pushed the enhance_grid_status branch from 65a2016 to b231f90 Compare July 10, 2018 18:22
@krmahadevan
Copy link
Contributor Author

@shs96c

I'd ask for a test of some description to ensure that this continues working....

I have now added required tests for this changeset and also addressed all the other review feedback. Please help take a look.

@krmahadevan krmahadevan force-pushed the enhance_grid_status branch from b231f90 to 4f9d089 Compare July 10, 2018 18:33
shs96c
shs96c approved these changes Jul 10, 2018
View reviewed changes
Copy link
Member

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I'll merge this now.

@shs96c shs96c merged commit 5289e97 into SeleniumHQ:master Jul 10, 2018
@krmahadevan krmahadevan deleted the enhance_grid_status branch July 10, 2018 18:43
grigaman pushed a commit to grigaman/selenium that referenced this pull request Jul 12, 2018
@krmahadevan
Enriching Hub Status to include Node info (SeleniumHQ#6127)
676e6e8 
Currently the end-point /grid/api/hub/status 
does not provide information about the nodes 
attached to it and also the busy/free status 
per browser flavor on each of the nodes.

Enriching the end-point to provide node info 
when invoked with the option 
/grid/api/hub/status?configuration=nodes

Sample payload as below:

{
  "nodes": [
    {
      "Id": "http://192.168.1.6:5555",
      "browsers": [
        {
          "browser": "safari",
          "slots": {
            "busy": 0,
            "total": 1
          }
        },
        {
          "browser": "chrome",
          "slots": {
            "busy": 0,
            "total": 5
          }
        },
        {
          "browser": "firefox",
          "slots": {
            "busy": 0,
            "total": 5
          }
        }
      ]
    }
  ],
  "success": true
}
@peterabbott
Copy link

peterabbott commented Aug 6, 2018 •
edited
Loading

This appears to be a breaking change when using an existing grid for < 3.14 as it is expected the enhanced metadata. A graceful fall back could have been nice as the update was getting pulled in transitively via other dependencies.

Just wanted to mention to save anybody else a day trying to figure out why the all their automated test suites start to fail :/

@krmahadevan krmahadevan mentioned this pull request Aug 10, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mach6 mach6 mach6 approved these changes

@shs96c shs96c shs96c approved these changes

Assignees
No one assigned
Labels
C-java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@krmahadevan @luke-hill @peterabbott @shs96c @mach6