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.

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

Bug: Lifetime player stats being returned with matches #145

Open
bguilder opened this issue Apr 11, 2017 · 15 comments
Open

Bug: Lifetime player stats being returned with matches #145

bguilder opened this issue Apr 11, 2017 · 15 comments

Comments

@bguilder
Copy link
Collaborator

bguilder commented Apr 11, 2017

We have been returning player stats within the match object. This object is actually life-to-date-of-match rather than life-to-now. It's super confusing.

The intention was to have snapshot info on the participant record. To fix this, we are considering removing the player data from the match object. We will be keeping participant data (data relevant to the match). However, in order to retrieve player lifetime data, the players endpoint will need to be used, and matches will no longer store lifetime player information.

If anybody foresees their apps breaking with this update, please let us know and we can try to accommodate. Thanks y'all!

@ClarkThyLord
Copy link
Contributor

This is gonna break multiple things in my bot, EZLBot. 😓 When is this change happening? So I can build a work around can't have a broken bot serving 500+ servers 😬

@turindev
Copy link
Contributor

Yup this will break my site as well, we will just need to coordinate. Also this will increase API calls so I'll have to consider the implications :)

@bguilder
Copy link
Collaborator Author

Yup, this is sure to break some sites. We'll definitely be sure everyone is notified and has time to make the necessary changes before deploying anything 👍

@schneefux
Copy link
Contributor

This will break our service in parts.
This means that in order to display a proper "players a b c played against d e f" view, we will need up to 5 * 50 + 1 requests per page from /matches, to get player's names.
Will our rate limits be increased accordingly?
Even with a limit of 50, there is no way to download all relevant data for one user within the average time of 1~2s it takes at the moment.

@PierreAndreis
Copy link
Contributor

Players Name are being stored on player data, while the participant data is only giving player id.
Does that mean I will have to make a call to /player endpoint to get the player names in a match? so 1 match with 6 participants = 1 match call + 5 players call ?

@svperfecta
Copy link
Contributor

Noted: We need to keep player.name somewhere to handle name changes.

@kvahuja
Copy link
Contributor

kvahuja commented Apr 12, 2017

we need playername, apiId at the least in same response to avoid making recurring calls

@PierreAndreis
Copy link
Contributor

@schneefux
Copy link
Contributor

@cklugewicz made a very good suggestion/comment, I'll quote it even if he/she decided to delete it:

The current /players endpoint only allows us to retrieve 1 player per call. I propose increasing that to 6, so that we can retrieve all players in a match with a single call.

Actually, this is a possible solution to this issue. player.id will be returned in relationships, and filter[playerIds] works on /players, it is currently undocumented:

http 'https://api.dc01.gamelockerapp.com/shards/eu/players' "Accept":"application/json" "X-Title-Id":"semc-vainglory" "Authorization":"" "filter[playerIds]"=="8f454688-2fe5-11e6-9433-06d90c28bf1a"
# 200

It does not work for an array of ids though:

http 'https://api.dc01.gamelockerapp.com/shards/eu/players' "Accept":"application/json" "X-Title-Id":"semc-vainglory" "Authorization":"" "filter[playerIds]"=="8f454688-2fe5-11e6-9433-06d90c28bf1a,727fa644-036e-11e7-8b59-062cb73cdbb1"
# 404

If we could get up to 6 players at once from /players, applications that need participant's stats could do it with 1 call per match, which is much more reasonable, especially if it happens on demand.


I have some concerns about including player in /matches' results. It is bad design to have more attributes for the same object (object type) on one endpoint than on the other. A player from /players would be !== a player from /matches. A clean solution to this could be using meta in relationships to store the player name (http://jsonapi.org/format/#document-meta), but I don't know enough about JSONAPI to want to recommend or suggest anything.
A reason why I'm opposed to encourage searching by player.attributes.name on /matches completely is that player names are not unique on this endpoint, but they are on the other; which means that data for a player vaingloryPlayer from /matches does not necessarily belong to the player vaingloryPlayer in the case that players changed their name - as far as I understand, data in /matches is "frozen" and has a snapshot of the player's name at that time, which does not mean that the player <-> id mapping is current or unique, which is misleading.
Either the API needs to map name to id internally before starting the search, which means returning matches that don't include the name that has been searched, or application developers need to map name to id via an additional call to /players first (which is not a good idea until #151 is solved).


tl;dr: API is in Alpha, this change will break applications anyways, better do more big breaking changes now than have more problems with player in the future.

@svperfecta
Copy link
Contributor

Still not sure what we want to do here. Still need to think about this more. Ideally I think the answer is that we return only the latest version of the player.

@svperfecta svperfecta removed this from Ready in API Aug 11, 2017
@schneefux
Copy link
Contributor

I have a solution:

  • You move all player.attributes to participant.attributes:
    • elo_earned_*, played*, wins, xp, *Streak
    • Especially name.
  • You remove type: player objects from the response as suggested here.
  • /matches?filter[playerNames] searches participant.attributes.name instead of player.attributes.name.

participant is already a record that contains player's data until the start of the match. You make the bug a documented feature.

@ghost
Copy link

ghost commented Aug 25, 2017

Yup. I suggest moving player.id (uuid) and player.name to participants and getting rid of players. Everything else is irrelevant from an immutable match perspective.

@svperfecta
Copy link
Contributor

@schneefux Yeah, I think that's a pretty good solution. We can do this. I'll add to the API list now.

@svperfecta svperfecta added this to Top 5 in API Sep 7, 2017
@svperfecta svperfecta added this to the VG 2.9 milestone Sep 7, 2017
@svperfecta
Copy link
Contributor

Tasks:

  • Move all player data to participant
  • Remove player from all match responses

@svperfecta svperfecta removed this from the VG 2.9 milestone Sep 7, 2017
@svperfecta
Copy link
Contributor

svperfecta commented Sep 7, 2017

This issue was moved to gamelocker/platform#8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
API
Top 5
Development

No branches or pull requests

9 participants