Skip to content

A Hub API that can query all running sessions. #6158

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
Merged
merged 2 commits into from
Jul 30, 2018
Merged

A Hub API that can query all running sessions. #6158

merged 2 commits into from
Jul 30, 2018

Conversation

krmahadevan
Copy link
Contributor

Closes #6070

The API can be accessed via
http://IP:Port/grid/api/sessions.

Both GET and POST are supported and result in the
same behavior.

A typical response can look like below (intentionally
trimmed the capabilities to reduce verbosity)

{
  "proxies": [
    {
      "proxyId": "http://192.168.1.6:5555",
      "proxyRemoteHost": "http://192.168.1.6:5555",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "df8aa16d-1d52-2e43-b584-2f9df586b0f3",
            "capabilities": {}
          },
          {
            "id": "f85f5641-e6de-3642-913a-a45aafdb85ad",
            "capabilities": {}
          }
        ]
      }
    },
    {
      "proxyId": "http://192.168.1.6:5556",
      "proxyRemoteHost": "http://192.168.1.6:5556",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "f4df4fbd-96ee-e042-ab43-40a33698d1e9",
            "capabilities": {}
          }
        ]
      }
    }
  ],
  "success": true
}

@krmahadevan
Copy link
Contributor Author

ping @shs96c @barancev - Can one of you please help take a look at this ?

shs96c
shs96c requested changes Jul 15, 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.

This is missing tests, so it'll be easy to introduce accidental regressions.

The approach taken here is very "bare metal": we're basically exposing the internals of the current grid to the world. Is this the approach we want to take, or would we prefer something that explains the domain better? I'm perfectly happy if the answer is "bare metal to get us off the ground", as I think we're allowed to iterate on Grid functionality with greater freedom than anything on the client side.

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Both of these code paths are the same....

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 it. Initially I was thinking of supporting something like giving the ability to query sessions given a node. Forgot to remove the branching, when I didn't go forward with the implementation.

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please put variable declarations on separate lines.

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/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

You have a json field declared. Why not use that?

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/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This is already going to be returned as a list of proxies. Having the word "proxy" seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current sessions payload, has no information about the proxy. That is why added both the proxyId and proxyRemoteHost so that the user gets to know as to for which proxy does the sessions info belong to.

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this, you can get the json coercer to do all the hard work:

Map<String, Object> body = json.newInput(reader).read(Json.MAP_TYPE);

Also, you're going to leak the JsonInput: best open that in the try statement above.

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to indicate some form of error has occurred?

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 it. The status gets flipped to false if there was any difficulty in extracting sessions info

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Heavens. Just call rsp.getContent(). You're not saving any memory doing this.

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. I think I borrowed that code from somewhere :)

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

And this is rsp.getContentEncoding()

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.

@krmahadevan
Copy link
Contributor Author

@shs96c

This is missing tests, so it'll be easy to introduce accidental regressions.

I was initially confused as to what sort of tests I add because this new servlet internally makes a call to WebDriverServlet running in the node. So didn't know how to fake that out. Ended up writing a E2E test which tests the functionality by spinning a grid hub and node.

The approach taken here is very "bare metal": we're basically exposing the internals of the current grid to the world. Is this the approach we want to take, or would we prefer something that explains the domain better? I'm perfectly happy if the answer is "bare metal to get us off the ground", as I think we're allowed to iterate on Grid functionality with greater freedom than anything on the client side.

The ask in #6070 was explicitly to have the capability of listing out all the sessions running in the Hub via an API. Please let me know if there's any other way of giving this capability. I will alter my PR accordingly.

I have addressed the review comments. Please help take a look.

@krmahadevan
Copy link
Contributor Author

@shs96c - thank you so much for merging the other two PRs. Can you please check if this one needs any additional work or if it can be merged as well ?

@p0deje p0deje added C-java Java Bindings B-grid Everything grid and server related and removed C-java Java Bindings labels Jul 26, 2018
@krmahadevan
A Hub API that can query all running sessions.
4a1fcaf 
Closes SeleniumHQ#6070

The API can be accessed via 
http://<IP:Port>/grid/api/sessions.

Both GET and POST are supported and result in the
same behavior.

A typical response can look like below (intentionally
trimmed the capabilities to reduce verbosity)

{
  "proxies": [
    {
      "proxyId": "http://192.168.1.6:5555",
      "proxyRemoteHost": "http://192.168.1.6:5555",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "df8aa16d-1d52-2e43-b584-2f9df586b0f3",
            "capabilities": {}
          },
          {
            "id": "f85f5641-e6de-3642-913a-a45aafdb85ad",
            "capabilities": {}
          }
        ]
      }
    },
    {
      "proxyId": "http://192.168.1.6:5556",
      "proxyRemoteHost": "http://192.168.1.6:5556",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "f4df4fbd-96ee-e042-ab43-40a33698d1e9",
            "capabilities": {}
          }
        ]
      }
    }
  ],
  "success": true
}
@krmahadevan
Fixing code review comments.
5f591bd
@krmahadevan
Copy link
Contributor Author

Rebased off of master and pushed in the updated branch.

@shs96c shs96c merged commit f3197b1 into SeleniumHQ:master Jul 30, 2018
@krmahadevan krmahadevan deleted the enrich_node_status branch July 30, 2018 14:26
@krmahadevan krmahadevan mentioned this pull request Aug 10, 2018
Open
grigaman pushed a commit to grigaman/selenium that referenced this pull request Sep 20, 2018
@krmahadevan
A Hub API that can query all running sessions. (SeleniumHQ#6158)
56acff7 
Closes SeleniumHQ#6070

The API can be accessed via 
http://<IP:Port>/grid/api/sessions.

Both GET and POST are supported and result in the
same behavior.

A typical response can look like below (intentionally
trimmed the capabilities to reduce verbosity)

{
  "proxies": [
    {
      "proxyId": "http://192.168.1.6:5555",
      "proxyRemoteHost": "http://192.168.1.6:5555",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "df8aa16d-1d52-2e43-b584-2f9df586b0f3",
            "capabilities": {}
          },
          {
            "id": "f85f5641-e6de-3642-913a-a45aafdb85ad",
            "capabilities": {}
          }
        ]
      }
    },
    {
      "proxyId": "http://192.168.1.6:5556",
      "proxyRemoteHost": "http://192.168.1.6:5556",
      "sessions": {
        "status": 0,
        "value": [
          {
            "id": "f4df4fbd-96ee-e042-ab43-40a33698d1e9",
            "capabilities": {}
          }
        ]
      }
    }
  ],
  "success": true
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shs96c shs96c shs96c requested changes

Assignees
No one assigned
Labels
B-grid Everything grid and server related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krmahadevan @shs96c @p0deje