Feature/#747 save additional metadata in game results

[eoinnoble]

Some third parties, and in future Galactic War, will send additional information with game result messages like

"phantom score -5"
"recall victory 5"

This PR captures those additional tags, tries to resolve them across multiple messages, and finally adds them to the TeamRatingSummary object(s) that we broadcast at the end of a game.

I'm not 100% happy with the naming of things, it feels like there are a lot of objects or enumerators named like *Result, *Score and *Info and I'm not sure if there are or should be clear distinctions about when to use each noun. I've also erred on the side of caution when resolving the metadata tags: if there's any discrepancy in the tags sent for an army then we'll only return the tags that exist in all messages. We may want to be more lenient than that.

Closes #747.

[codecov]

Codecov Report

Merging #782 (ad16ad8) into develop (38cc8fe) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted Files	Coverage Δ
server/rating_service/typedefs.py	95.45% <ø> (ø)
server/gameconnection.py	94.34% <100.00%> (-0.68%)	⬇️
server/games/game.py	95.64% <100.00%> (+0.04%)	⬆️
server/games/game_results.py	98.70% <100.00%> (+0.19%)	⬆️
server/games/typedefs.py	98.26% <100.00%> (+0.01%)	⬆️
server/protocol/gpgnet.py	90.00% <0.00%> (-5.00%)	⬇️
server/timing/timer.py	77.77% <0.00%> (-1.59%)	⬇️

[Askaholic]

This is a great start! I think some things can actually be simplified a little bit.

After reading this PR, I think the feature boils down to 3 chunks of work that we should tackle in the following order:

1. Save the metadata on the GameResultReport object instead of ignoring them
2. Include the metadata in the EndedGameInfo object
3. Add conflict resolution for metadata

So I'll probably try to focus my reviews in that order as well.

We'll definitely need some integration tests, but it may be easier to write those with the changes from my broadcast PR.

[Askaholic]

Maybe this is beyond the scope of this PR, but I think we actually want these rabbitmq messages to go out even for games that are not marked as VALID. In the case of GW I would expect many games to be invalid either due to mods or unbalanced teams.

If this complicates things too much for you don't worry about it for now, we'll make an issue and do it separately.

[eoinnoble]

Ah ok, I'll look into it and see if I can manage it.

[eoinnoble]

Have taken a stab at it, let me know what you think?

[Askaholic]

For some reason I thought the messages wouldn't be sent to rabbitmq if the game wasn't valid, but looking over the code again I don't think that's the case. GameService.publish_game_results always publishes to rmq, and only sends to the rating service if the game wasn't valid, which is the behavior we want.

So I think you can just remove that if self.validity is VALID check and everything will work as expected. Definitely want a test or two to confirm tho.

[eoinnoble]

Hmm, ok, will take a look at this.

[Askaholic]

Yes! This looks really good now; much simpler than it was at first.

Lastly I'd like to see some integration tests, which means I guess I had better get a move on with my rabbitmq PR. You could also copy the AioQueueProtocol helper class and then rebase again once it's merged. Or maybe this will be merged first and then I can rebase my PR, doesn't really matter.

[Askaholic]

So looks like you are gonna keep this in for now?

[eoinnoble]

Yeah I thought this might be better as a separate PR as it looked like I might have to alter the logic of game_results.resolve_game as well and we might want to be able to roll that back easily without undoing the changes in this PR?

[Askaholic]

Alright, I'm merging this. Here's something quirky I noticed, but it's only because we're using python3.7.

When the end game result is logged, it prints out the object before serialization (no problem this is actually nice for printing enums) and in python3.7 the return value of NamedTuple._asdict is an OrderedDict which prints out kinda ugly:

TRACE    May 29  22:23:45 MessageQueueService            Published message {'game_id': 13757642, 'rating_type': 'global', 'map_id': 558, 'featured_mod': 'faf', 'sim_mod_ids': [], 'commander_kills': {}, 'validity': 'VALID', 'teams': [{'outcome': 'DEFEAT', 'player_ids': [59632], 'army_results': [OrderedDict([('player_id', 59632), ('army', 1), ('army_outcome', 'DEFEAT'), ('metadata', ['recall'])])]}, {'outcome': 'VICTORY', 'player_ids': [273862], 'army_results': [OrderedDict([('player_id', 273862), ('army', 2), ('army_outcome', 'VICTORY'), ('metadata', [])])]}]} to faf-lobby/success.gameResults.create

However, in python3.8 the return value is just a normal dict so lets just leave it. (I'll have to give upgrading another shot at some point)