Adopt Resource based APIs for Player Tracking

[domgreen]

Is your feature request related to a problem? Please describe.
I think it would be benificial to adopt resource based APIs for the alpha features of Player tracking following the google cloud api design docs.

Describe the solution you'd like
I have looked at each resource that is currently exposed via Player tracking and have an alternative suggestion for the API documented below:

• Alpha: PlayerConnect

  • $ curl -d '{"playerID": "uzh7i"}' -H "Content-Type: application/json" -X POST http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/connect

• Proposed

  • $ curl -d '{"playerID": "uzh7i"}' -H "Content-Type: application/json" -X POST http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players
  • 200 {name="players/uzh7i", "playerID": "uzh7i"}

• Alpha: PlayerDisconnect

  • $ curl -d '{"playerID": "uzh7i"}' -H "Content-Type: application/json" -X POST http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/disconnect

• Proposed

  • $ curl -X DELETE http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players/uzh7i
  • 404

• Alpha: SetPlayerCapacity

  • $ curl -d '{"count": 5}' -H "Content-Type: application/json" -X PUT http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/capacity

• Proposed

  • $ curl -d '{"count": 5}' -H "Content-Type: application/json" -X PUT http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/capacity
  • 200 {"count": 5}

• Alpha: GetPlayerCapacity

  • $ curl -d '{}' -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/capacity

• Proposed

  • $ curl -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/capacity

• Alpha: GetPlayerCount

  • $ curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/count

• Proposed

  • $ curl -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players
  • 200 {"total_size":2, "players":[{name="players/uzh7i", "playerID": "uzh7i"},{name="players/3zh7i", "playerID": "3zh7i"}]}

• Alpha: IsPlayerConnected

  • $ curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/connected/uzh7i
  • {"bool":true}

• Proposed

  • $ curl -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players/uzh7i
  • 200 {name="players/uzh7i", "playerID": "uzh7i"}

• Alpha: GetConnectedPlayers

  • $ curl -H "Content-Type: application/json" -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/alpha/player/connected
  • {"list":["uzh7i","3zh7i"]}

• Proposed

  • $ curl -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players
  • 200 {"total_size":2, "players":[{name="players/uzh7i", "playerID": "uzh7i"},{name="players/3zh7i", "playerID": "3zh7i"}]}

Notice that the majority of actions hang off /players and /players/<id>, the one that I struggled to see how it fitted cleanly was capacity.

Doing this also reduces the api suface area to just deal with the resources, IsConnected is just a check to see if the expected resource is returned, assume it isnt connected if a 200 is not returned.

Describe alternatives you've considered
Keep existing implementation is a valid alternative.

Additional context
NA

[markmandel]

Overall this sound like a good approach! a few thoughts:

➡️ http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/players

Since I can't see how we would ever have a v1alpha2 (we've no guarantee of backward compat between alpha versions), maybe we should settle on v1alpha instead?

Having said that, according the docs that I wrote 😆 (https://agones.dev/site/docs/guides/feature-stages/#rest--grpc-apis), we should be using v1alpha1 😕 WDYT though?

➡️ players vs player

Looking at:
https://cloud.google.com/apis/design/standard_methods#create
https://cloud.google.com/apis/design/naming_convention

We should be using /players/ across the board.

➡️ curl -X GET http://localhost:${AGONES_SDK_HTTP_PORT}/v1alpha1/capacity

Since "capacity" is a fairly generic term, and if we have /players/ as the main root, should we go to /players/capacity as resource the endpoint? Then it's all under the players category.

WDYT?

[domgreen]

Versioning:

• No strong opinion. I followed this https://cloud.google.com/apis/design/versioning that suggests if you do another release of the alpha you should increment till it is moved into beta / stable.

players vs player:

• agree players across the board

capacity:

• this is a tough one ... is /players/capacity. I'm not against the idea but if we do it with capacity should we follow with count?
• alternatively, lean more towards the count suggestion where calling /players gives back the count and capacity in the body along with a list of players

/players
{
  capacity: 10,
  count: 2,
  players: [
  ...
  ]
}

[markmandel]

@pooneh-m you always have excellent thoughts on versioning and structure - WDYT?

[pooneh-m]

Thanks @markmandel. About versioning, I think increasing the v1alpha version should not be mandatory on per API changes, but per impact. For example if suddenly the behavior of a field is changed or there is a wide usage of the alpha fields that needs breaking changes (e.g. k8s API), then you upgrade to v1alpha(N+1). This is to give heads up that something is changed radically and force users to read through documentation before usage, which is more common in beta and uncommon in alpha.

[markmandel]

Thanks! I think we have consensus on keeping v1alpha1 then!

alternatively, lean more towards the count suggestion where calling /players gives back the count and capacity in the body along with a list of players

Ah yes, we should have a separate GET /players and GET /players/count -- mainly because I don't think gRPC gives us a way to aggregate that information together -- they are separate services.

So I would argue to have /players/count and /players/capacity respectively 👍

[pooneh-m]

I've been looking into this proposal again.

Player capacity or player count is for a game session. Should we introduce them as part of a game session property vs player property? count would be the state of a game session.

[domgreen]

Interesting ... what would this look like as an API?

/v1alpha1/gamesessions/capacity? < Can PUT/GET this
/v1alpha1/gamesessions/count < Can GET this

This now adds the idea of sessions to Agones though that might be a bigger discussion 🤔

But agree with the idea that if it is resource based the players collection doesnt really own the capacity.

[pooneh-m]

Capacity is a property and should be in the body of the request:
It would be like

GET <endpoint>/v1alpha1/gamesessions/<name>
{
  "name": <game-session-resource-name>,
  "players_capacity": 10,
  "status": {
    "active_players": 2, #instead of count - can it be inferred from players list below? if so it should be dropped
    "players": [ # Instead of list of ids players should be extendable to include player properties, such as active, expertise, etc
        {
            "player_id": "p1",
            "...",
        },
        {
            "player_id": "p2",
        }
    ],
  }
}

PlayerConnect would be a custom method I think: https://google.aip.dev/136.
Also, this is a great resource for designing APIs: https://google.aip.dev/

[pooneh-m]

This now adds the idea of sessions to Agones though that might be a bigger discussion 🤔

I think we should introduce the concept early and initially support one gamesession? In future when multiple gamesessions are supported, aggregation can happen in the client side by listing the gamesessions. Introducing an API that does the aggregation would be an advanced topic and only supported if needed due to performance or complexity.

[markmandel]

Since the gRPC endpoint don't support sessions - should we be doing this at the REST level?

[pooneh-m]

Since the gRPC endpoint don't support sessions - should we be doing this at the REST level?

I was providing an alternative option for the REST resource proposal to use gamesession resource with player as a sub-resource instead.

[markmandel]

I was providing an alternative option for the REST resource proposal to use gamesession resource with player as a sub-resource instead.

Gotcha, but what would be in that context? Is there implied functionality in the REST endpoints that doesn't exist in the gRPC service (seems to me like there is)?

I'm also trying to consider what's technically possible with the grpc-gateway to expose the gRPC endpoints. I'm not sure how much we can manipulate the grpc-gateway's exposure of the gRPC services.

Unless we are saying that we should change the gRPC services?

Since we've also not done any design work on #1197 - should we be jumping into this now?

[pooneh-m]

There seems to be a problem with the way resources are designed. At here

For example, in the following the REST API:

    rpc PlayerConnect (PlayerID) returns (Bool) {
        option (google.api.http) = {
            post: "/alpha/player/connect"
            body: "*"
        };
    }

According to https://google.aip.dev/144 it should be:

    rpc AddPlayer (AddPlayerRequest) returns (AddPlayerResponse) {
        option (google.api.http) = {
            post: "/v1alpha1/players:addPlayer"
            body: "*"
        };
    }

My other point was, it seems like the player is subresource of the gamesession.

Gotcha, but what would be in that context? Is there implied functionality in the REST endpoints that doesn't exist in the gRPC service (seems to me like there is)?

I can't think of anything.

I'm also trying to consider what's technically possible with the grpc-gateway to expose the gRPC endpoints. I'm not sure how much we can manipulate the grpc-gateway's exposure of the gRPC services.

Unless we are saying that we should change the gRPC services?

We should fix the design with the API. It is not following the best practices. I think REST would be exposed on the same endpoint, but different port using grpc-gateway.

Since we've also not done any design work on #1197 - should we be jumping into this now?

We should consider how it would look like and whether gamesession should be a parent resource.

[domgreen]

That site is fantastic 😄 thank you for sharing that, as is the linter that goes along with it.

@pooneh-m OOI why do you prefer a custom method over using the create as a connection? With the create we can supply the player_id as suggested in https://google.aip.dev/133 under User-specified IDs.

@markmandel how would the creation of sessions effect PlayerTracking? Does this need to be thought of before PlayerTracking leaves alpha? We could I suppose have player tracking return for both the whole gameserver and the gamesession. Which leads me to think that the capacity and count of hte current PlayerTracking hangs off game server not players.

Sorry if I've open a can of worms here :)

[markmandel]

Sorry if I've open a can of worms here :)

Ha! The best discussions come from a can of worms, I feel 😄

100% agreed with @pooneh-m sharing https://google.aip.dev, it's awesome! Thank you for sharing it! I wish we'd seen it at the beginning. Maybe we should link to this in https://github.com/googleforgames/agones/blob/master/CONTRIBUTING.md#formatting ? Make it a standard (that we will attempt to keep as best we can, since we didn't conform from the start)?

Re: PlayerConnect - that's an interesting question then. Is it a custom method, or a repeated field, or a resource in it's own right that be created and deleted? I would argue it's either a custom method (since it both adds, and increments a counter), or a repeated field (contained in the GameServer).

Re: game session vs server -- my thinking is to keep things simple for now, and focus on the server. When it comes back around to how we look at game sessions, there are so many cross cutting concerns, I'm hesitant to touch it piecemeal, without some overarching design work to look at the problem holistically.

[domgreen]

100% agreed with @pooneh-m sharing https://google.aip.dev, it's awesome! Thank you for sharing it! I wish we'd seen it at the beginning. Maybe we should link to this in https://github.com/googleforgames/agones/blob/master/CONTRIBUTING.md#formatting ? Make it a standard (that we will attempt to keep as best we can, since we didn't conform from the start)?

Yes that's a great idea should aim to adhere with all new services.

Re: PlayerConnect - that's an interesting question then. Is it a custom method, or a repeated field, or a resource in it's own right that be created and deleted? I would argue it's either a custom method (since it both adds, and increments a counter), or a repeated field (contained in the GameServer).

I see it as a resource initially as the presence of the resource is the player being connected. Would you ever have a player not connected on the collection? But as you both point out it updating the separate resource for capacity/count is interesting, will sleep on this one and see what you both come up with 😁

Re: game session vs server -- my thinking is to keep things simple for now, and focus on the server. When it comes back around to how we look at game sessions, there are so many cross cutting concerns, I'm hesitant to touch it piecemeal, without some overarching design work to look at the problem holistically.

Agree, lots of spikey bits with multiple sessions. Would need alot of thinking to get correct for majority of scenarios.

[domgreen]

So having a look over my new favourite site https://google.aip.dev/ I think i am leaning more to /players being a resource.

Connect is just a create of the player resource, disconnect is similarly just a delete, and connected is similar as well with get:

// https://google.aip.dev/133
message CreatePlayerRequest {
  Player player = 2 [(google.api.field_behavior) = REQUIRED];

  // we supply our own ID in the create
  string player_id = 3;
}

List will get all players back (paged if needed) with a total_size reflecting the total size of the collection (players). we can also add the capacity_count here as a custom method on the collection.

I would argue this wouldn't be a repeated field as it is not a primitive and may exceed 100 entries.

// https://google.aip.dev/132
message ListPlayersResponse {
  repeated Player players = 1;

  string next_page_token = 2;
  
  int32 total_size = 3;

  int32 capacity_count = 4;
}

As mentioned above I would move capacity to be a custom method on the players collection for the moment to fit in with playertracking being a seperate thing the GS and not wanting to tackle sessions. See https://google.aip.dev/136 and collection based methods could also have a get here.

rpc SetCapacityPlayers(SetCapacityPlayersRequest) returns (SetCapacityPlayersResponse) {
  option (google.api.http) = {
    post: "/v1alpha1/players:setcapacity"
    body: "*"
  };
}

Happy with alternate suggestions just my 2p that most of these calls from a HTTP1 standpoint will probably want to hang off players.

[markmandel]

So I took a stab at refactoring the alpha.proto to work on these changes - specifically on treating Player as a resource.

For right now I've completely ignored get/set capacity -- I just wanted to make sure I was on the right track first, and will then look at tackling that aspect.

It has come up on this issue around managing game-sessions. Given the direction of #1239 - I pretty strongly think we won't implement functionality to track individual game sessions, and instead punt that up to the game server binary, and therefore shouldn't be concerned about that aspect within this API. Or to put it another way /players is a top level resource.

// Copyright 2020 Google LLC All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package agones.dev.sdk.alpha;
option go_package = "alpha";

import "google/api/annotations.proto";

// A representation of a book in the library.
message Player {
    option (google.api.resource) = {
        type: "agones.dev/Player"
        pattern: "players/{player}"
    };

    // The resource name/unique player id for the player.
    // Formatted as `players/{ident}` where {ident} is the provided player identifier.
    string name = 1;
}

message CreatePlayerRequest {
    Player player = 1 [(google.api.field_behavior) = REQUIRED];
    string player_id = 2 [(google.api.field_behavior) = REQUIRED];
}

message DeletePlayerRequest {
    string name = 1 [
                    (google.api.field_behavior) = REQUIRED,
                    (google.api.resource_reference) = {
          type: "agones.dev/Player"
        }];
}

message ListPlayersRequest {
    // The maximum number of players to return. The service may return fewer than
    // this value.
    // If unspecified, at most 1000 players will be returned.
    int32 page_size = 1;
    string page_token = 2;
}

message ListPlayersResponse {
    repeated Player players = 1;
    string next_page_token = 2;
    int32 total_size = 3;
}

message GetPlayerRequest {
    string name = 1 [
                    (google.api.field_behavior) = REQUIRED,
                    (google.api.resource_reference) = {
      type: "agones.dev/Player"
    }];
}

// SDK service to be used in the GameServer SDK to the Pod Sidecar.
service SDK {
    // CreatePlayer increases the SDK’s stored player count by one, and appends this Player's unique name to GameServer.Status.Players.IDs.
    //
    // GameServer.Status.Players.Count and GameServer.Status.Players.IDs are then set to update the player count and id list a second from now,
    // unless there is already an update pending, in which case the update joins that batch operation.
    //
    // CreatePlayer returns the Player and adds the Player to the list of Players if this Player was not already in the
    // list of connected Players.
    //
    // If the Player exists within the list of connected Players, CreatePlayer will return the error ALREADY_EXISTS, and the list of
    // connected Players will be left unchanged.
    //
    // The error OUT_OF_RANGE will be returned if the Player was not already in the list of connected Players but the
    // player capacity for the server has been reached. The Player will not be added to the list.
    //
    // Warning: Do not use this method if you are manually managing GameServer.Status.Players.IDs and GameServer.Status.Players.Count
    // through the Kubernetes API, as indeterminate results will occur.
    rpc CreatePlayer(CreatePlayerRequest) returns (Player) {
        option (google.api.http) = {
            post: "/v1alpha1/players"
        };

        option (google.api.method_signature) = "player";
    }

    // Decreases the SDK’s stored player count by one, and removes the Player from GameServer.Status.Players.IDs.
    //
    // GameServer.Status.Players.Count and GameServer.Status.Players.IDs are then set to update the player count and id list a second from now,
    // unless there is already an update pending, in which case the update joins that batch operation.
    //
    // DeletePlayer will succeed and remove the supplied Player from the list of connected Players if the
    // Player's value exists within the list.
    //
    // If the Player was not in the list of connected Player, the call will return error with a status of NOT_FOUND,
    // and the connected Player list will be left unchanged.
    //
    // Warning: Do not use this method if you are manually managing GameServer.status.players.IDs and GameServer.status.players.Count
    // through the Kubernetes API, as indeterminate results will occur.
    rpc DeletePlayer(DeletePlayerRequest) returns (google.protobuf.Empty) {
        option (google.api.http) = {
            delete: "/v1alpha1/{name=players/*}"
          };
        option (google.api.method_signature) = "name";
    }

    // Returns the list of the currently connected Players. This is always accurate from what has been set through this SDK,
    // even if the value has yet to be updated on the GameServer status resource.
    //
    // The current player count is included in the response. This is always accurate from what has been set through this SDK,
    // even if the value has yet to be updated on the GameServer status resource.
    //
    // If GameServer.Status.Players.IDs is set manually through the Kubernetes API, use SDK.GameServer() or SDK.WatchGameServer() instead to view this value.
    rpc ListPlayers(ListPlayersRequest) returns (ListPlayersResponse) {
        option (google.api.http) = {
            get: "/v1alpha1/players"
        };
    }

    // Get the Player. This is always accurate from what has been set through this SDK,
    // even if the value has yet to be updated on the GameServer status resource.
    // If the Player does not exist, then an error of NOT_FOUND is returned.
    //
    // If GameServer.Status.Players.IDs is set manually through the Kubernetes API, use SDK.GameServer() or SDK.WatchGameServer() instead to determine connected status.
    rpc GetPlayer(GetPlayerRequest) returns (Player) {
        option (google.api.http) = {
            get: "/v1/{name=players/*}"
        };
        option (google.api.method_signature) = "name";
    }

   // ... capacity pieces removed ....

}

What do you think? I will also run the linter over this once we're in a reasonably happy place, and then I need to work out how we are going to actually implement all this in one go. 😃 As far as I can tell, the API surface here will still enable the SDK API to be able to function in the same way it does currently - with the only difference I can think of, maybe being the List functionality (since this new proto has pagination 🤔 maybe the default for list operation max size should be the current capacity??? 🤔 ).

Here is a link, if you want to see a comparison between the old version and the new

[domgreen]

For right now I've completely ignored get/set capacity -- I just wanted to make sure I was on the right track first, and will then look at tackling that aspect.

Fair, initial thoughts here would be a custom method on the players collection https://google.aip.dev/136

It has come up on this issue around managing game-sessions. Given the direction of #1239 - I pretty strongly think we won't implement functionality to track individual game sessions, and instead punt that up to the game server binary

Agree this can easily be implemented via labels if you need to know the capacity/population of a specific instance within the DGS.

Question about Player/name field ... currently player names take the form based on anything the DGS gives eg. abc123 would you be looking to prepend it with players/ to make it players/abc123 which meets the AIP https://google.aip.dev/122 I don't think its a big issue either way but we should be clear in the documentation what the result will be.

With the List RPC I don't think its a issue ... we are very unlikely (correct me if I'm wrong) that we will see 1000 (the default) players per DGS so could probably get away without pagination and leaving the fields there to satisfy the linter but clear in the documentation that pagination is not currently implemented.

On the List I would also be tempted to add players_count and players_capactiy (or similar names) so that you then don't have to make a subsequent call to the other endpoints.

[markmandel]

abc123 would you be looking to prepend it with players/ to make it players/abc123 which meets the AIP https://google.aip.dev/122

Yeah, I think that would make sense. I added a note on formatting above.

With the List RPC I don't think its a issue ... we are very unlikely (correct me if I'm wrong) that we will see 1000 (the default) players per DGS so could probably get away without pagination

Actually, I think we will need pagination! For doing a SDK.GetPlayerCount() you can do a ListPlayers with a page_size of 0 , and use the total_size to provide the result.

Fair, initial thoughts [capacity] would be a custom method on the players collection https://google.aip.dev/136

On first pass that looks right to me too, I'll take a stab at it though, and update the design above.

Thanks for the feedback!

[markmandel]

This has been floating around for a while, and is a blocker on moving PlayerTracking into beta at some point, but I'm going to focus on Autoscaling Fleet base on Player Count #1034 first, so we have complete player tracking, allocation and autoscaling solution in place, then we are able to get feedback on the whole used in conjunction rather than each part individually - especially while they are in alpha/beta state.

Let me know if you have any thoughts / objections.

[github-actions]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[markmandel]

Should we close this now at wontfix, since we're already working on #2716 ?

[github-actions]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[markmandel]

Closing as wontfix.