Send raid lobbies to webhook #124

Mygod · 2023-06-26T17:24:03Z

This sends all raid lobbies directly to webhook and skips persisting to DB. Adds a new webhook type raid_lobby. Fixes #107.

Depends on #123. Untested but comments welcome.

This sends all raid lobbies directly to webhook and skips persisting to DB. Adds a new webhook type raid_lobby. Fixes UnownHash#107.

jfberry · 2023-06-26T17:27:03Z

I have only looked briefly so this could be inaccurate. This probably generates webhooks even with no change in the lobby value which is probably excessive.
If I am right (and have not missed the logic), then it would be easy enough to store the current value in the gym record in any case - just we wouldn't load or save it to the db. It would then also be available to be returned by any in-memory query later.

jfberry · 2023-06-26T17:28:29Z

A new webhook of this type (without broader gym details) would not be one poracle could notify on - it would need to be an extension to the one of the standard ones (or, at least have complete enough information to build a notification).
If it is not relevant to poracle to consume this then you can ignore this comment

ccev · 2023-06-26T18:36:47Z

if you're making this a complete raid webhook type, i think it should also be sent based on GMO data, with the lobby count being null

Mygod · 2023-06-26T18:39:22Z

Yes in that case those two fields would simply be missing from the webhook object. By the way, feel free to push directly to this PR. :)

main.go

Co-authored-by: Naji Astier <[email protected]>

Mygod · 2023-06-29T16:58:37Z

@jfberry Cached lobbies in memory as requested.

jfberry · 2023-06-29T17:19:21Z

This is looking really good. We just need a source of the data to be able to test, but will be merged when we can show it working correctly.

jfberry · 2023-06-29T17:11:31Z

decoder/gym.go

@@ -374,11 +376,77 @@ func createGymFortWebhooks(oldGym *Gym, gym *Gym) {
 	}
 }

+func makeRaidWebhook(gym *Gym) (payload map[string]interface{}) {


The main advantage (as I understand it) of using named returns is that you have immediately created the return type. You then go on and re-assign it, which gives rise to an additional allocation rather than having pre-made one.

Whilst this is not the world's biggest thing, I haven't wrapped my head around when it is good stylistically to use this, vs not recommended.

Even if that is not already optimized by the compiler already, a map allocation should still be virtually for free.

I expect you're right, it was more a style discussion. It seems the go language recommendations are only to use it on short functions where you can see the definition and the return on a single page, and warning against re-using the variable because it can be shadowed.

jfberry · 2023-06-29T17:12:30Z

decoder/gym.go

+		"power_up_end_timestamp": gym.PowerUpEndTimestamp.ValueOrZero(),
+		"ar_scan_eligible":       gym.ArScanEligible.ValueOrZero(),
+	}
+	if gym.LobbyPlayerCount > 0 {


the convention for all webooks webhooks would normally to report a null when there is no value; to do this you would need to use a func like above and return nil vs the integer.

Here 0 essentially means null. Do you want to always populate 0 instead?

If it is actually 0, then that makes sense. If you don't know, then it should be null.
I'm not really sure what this will look like with real data, so it might be better to hold this one over until we have a data source.

I wonder if it should be null until you have received the first lobby for a gym; then when the lobby times out go to 0? or go back to null?

Personally I think it's fine to keep it 0 always to keep it simple (instead of having to deal with 0 vs null), due to the extremely ephemeral nature of raid lobbies (only lasting at most 110 seconds).

Even if there was a lobby, it might never get scanned when it was active. Therefore to me, it seems pointless to distinguish whether the lobby was last seen empty/expired vs the lobby was never scanned since raid started.

I am thinking about downlevel users of the raid webhook. If we are sending old lobby details that's a bit pointless, eh? Any users of the data in eg a poracle notification would therefore see a lobby that wasn't actually correct.

Poracle can check if the lobby expired when it actually gets to it in the queue.

That's not how poracle works. The webhook information is available to users to put in their messages without processing; with some additions. We can hold this open until we get a data source and some implementors, but the current implementation is inconsistent with every other webhook

If you don't like it you have the freedom to change it.

jfberry · 2023-06-29T17:14:10Z

decoder/gym.go

+	}
+	gym.LobbyPlayerCount = lobby.PlayerCount
+	gym.LobbyJoinEndMs = lobby.LobbyJoinUntilMs
+	gymCache.Set(gym.Id, *gym, ttlcache.DefaultTTL)


I see what you're doing here - reproducing the change logic in save. I wonder if this is the clearest way (I'm not objecting here, just musing). The other alternative would be to call saveRecord as normal and have it decide there is no need to commit to database (ie there are no record changes), but still worth sending a webhook since there are raid lobby changes.

This seems like the best option for now.

Mygod · 2023-07-21T19:29:35Z

Merge when? 😃

jfberry · 2023-07-21T19:31:07Z

Merge when? 😃

Not until it’s been possible to test with real data to see that it works as we would wish - and to give opportunity to evaluate that the format and sequence of data is useful to any possible downstream users

jfberry · 2023-07-23T13:07:08Z

It sounds some people are now feeding this with data, please can you post a webhook sequence generated so we can see how lobbies evolve from egg onwards (eg null/vs 0/repeat entries) so we can consider if the format is correct for downstream implementers.

na-ji · 2023-07-25T15:47:25Z

Could you add GetRaidLobbyCounterOutProto to the list of supported protos in protos.md please?

Mygod · 2023-07-26T18:06:39Z

Hmm. Maybe this should be gated behind a config for webhook?

jfberry · 2023-07-26T18:11:53Z

Hmm. Maybe this should be gated behind a config for webhook?

If we aren't sending it by webhook, perhaps we shouldn't be processing it at all (since that's our only use case; maps can't get it yet because it is held in memory only). If there is an option it should probably just not process it (and therefore not generate the webhook).

Should this be added to 'scan context'? This is our generalised way of controlling what is processed, but this is location and scan mode related and I'm not sure either of these apply.

if later we manage to make a subscription based channel for changes to map inside a specific window (which would be possible with our geographic data structure and grpc support) we can consider whether the changes are pushed this way and optionally not by webhook - but we don't have that for now.

I have to say when you start to multiply up the number of data items and frequency of change (every 5-10 seconds for every active raid on a big map) it starts to look quite large quite quickly. As in discord, I fear for the way poracle does matches - but we won't really know until someone experiments with it.

Mygod · 2023-07-27T18:42:51Z

Just wondering. Is it more reasonable to put this switch here, or put it in the controller? If the controller does not send the queries then Golbat wouldn't process either.

jfberry · 2023-07-27T18:43:46Z

Most mitm won’t allow selectivity. It will come regardless from eg atlas

Mygod · 2023-07-27T18:50:03Z

Any suggestion of what the config should be? I don't see any config group that turns on/off certain processing... (Maybe it should be in a separate PR?)

jfberry · 2023-07-27T18:58:43Z

This is scan context, but as I say this is a per location / scan type config. This should probably just be a global setting to begin with.

Fabio1988 · 2023-08-14T07:48:11Z

This is conflicting with main now... But I would say sending raid lobbies to webhooks is getting more useless than before. with actual changes a lobby closes immediately if 20 players are in there, even 3-5 players are leaving again, nobody can join anymore.

So I would prefer to have some stats on it, rather than webhook notifications :)

Mygod · 2023-08-14T07:50:06Z

If you're at a place with constant 20-player lobbies, this PR is useless either way anyways?

jfberry · 2023-08-14T07:52:07Z

We should keep this open until a consumer exists or clear use arises as all of the content is valid just without a user yet

Mygod · 2023-08-14T07:53:01Z

Hi I'm a user. I like to help out my rural strangers with legendary raids. 🫡

jfberry · 2023-08-14T07:56:46Z

Hi I'm a user. I like to help out my rural strangers with legendary raids. 🫡

I mean a user of the webhooks as you well know. There is a lot to test about throughout and delivery timeliness to understand whether this has any practical use.

Prototypes have revealled it probably isn’t actually of any benefit so until someone invests the time then leaving this as a PR to support third party development should it happen makes most sense to me. The alternative would be to close it unmerged, but I wouldn’t want to abandon this just yet.

Mygod · 2023-08-14T07:58:42Z

That's what I meant? The use case is to track gyms near you that you could join immediately if a lobby starts.

jfberry · 2023-08-14T08:12:30Z

That's what I meant? The use case is to track gyms near you that you could join immediately if a lobby starts.

Indeed; but given the time remaining we have seen on real life hooks, processing & notification delay and no tooling yet to actually deliver this - combined with now 'ready to raid' shortening lobbies - I remain skeptical that a useful implementation will appear. However, simply because it's possible that it may I am suggesting we keep this open rather than close it unmerged.

Mygod · 2023-08-14T08:15:01Z

I feel like the norm for rural raiding is to keep going into lobbies and jump out until a critical mass of players are in the lobby. Whatever data you gathered at large cities will never be relevant since this feature is completely useless there to begin with.

Mygod · 2023-08-14T08:17:28Z

So maybe in order to decrease load, maybe downstream could instead use a grpc to let golbat know what gyms they're interested in raiding, and golbat will skip all other gym lobbies. The hook would also expire once the raid finishes or downstream unsubscribes. This way it should solve the issue of overloading poracle?

jfberry · 2023-08-14T08:24:39Z

Rural areas tend to have less frequent 'drive bys' by MITM scanners so actually catching a lobby is going to be less likely. As I said, I'm happy to leave this as a PR until others prototype solutions - my work on this subject (including sending the protos to golbat across my entire area) has lead me to believe that a useful & practical solution is unlikely to be forthcoming.
Should one exist then we will be able to evaluate load across the system in production and whether the webhook contains the right information etc.
You are right that an entirely different communication mechanism could modifies some of the timeliness (but that's only 1 second of the delay probably), or could provide a different subscription model than exists for current webhooks. It's unlikely to be poracle that would subscribe because that's a fundamentally different architecture.
I have seen an alternative prototype which receives the raws directly (no golbat needed); posts to discord for specific gyms and even that shows that given scan rates and discord push notification delays it's not quick enough to be useful.
Perhaps some dedicated application using android or apple push could get there fast enough if you increased scanning density.

Send raid lobbies to webhook

a7d55b3

This sends all raid lobbies directly to webhook and skips persisting to DB. Adds a new webhook type raid_lobby. Fixes UnownHash#107.

Mygod added 2 commits June 26, 2023 14:01

Change webhook to use raid type

812d336

Undo changes to gym details

518e959

na-ji reviewed Jun 26, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

na-ji linked an issue Jun 26, 2023 that may be closed by this pull request

Process GetRaidLobbyCounterOutProto #107

Open

Mygod and others added 2 commits June 26, 2023 18:34

Update main.go as suggested by @na-ji

d9b8c68

Co-authored-by: Naji Astier <[email protected]>

Cache lobbies in memory

ad3b092

jfberry reviewed Jun 29, 2023

View reviewed changes

Mygod marked this pull request as ready for review July 11, 2023 17:02

Merge branch 'main' into raidlobby

0bc02ef

Mygod and others added 4 commits July 23, 2023 11:44

Update routes.go to add log

137f4bb

Merge branch 'newproto' into raidlobby

ba4419d

Always send lobby info in webhook

f1e6b64

Update routes.go

563b0fb

Add Method_METHOD_GET_RAID_LOBBY_COUNTER to protos.md

3b8e4e3

Mygod changed the base branch from newproto to main July 26, 2023 21:35

Merge branch 'main' into raidlobby

94c1408

Add ProcessLobbies to scanContext

686f2ed

Send raid lobbies to webhook #124

Are you sure you want to change the base?

Send raid lobbies to webhook #124

Conversation

Mygod commented Jun 26, 2023 • edited Loading

jfberry commented Jun 26, 2023

jfberry commented Jun 26, 2023

ccev commented Jun 26, 2023 • edited Loading

Mygod commented Jun 26, 2023

Mygod commented Jun 29, 2023

jfberry commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mygod Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mygod commented Jul 21, 2023

jfberry commented Jul 21, 2023

jfberry commented Jul 23, 2023

na-ji commented Jul 25, 2023

Mygod commented Jul 26, 2023

jfberry commented Jul 26, 2023

Mygod commented Jul 27, 2023

jfberry commented Jul 27, 2023

Mygod commented Jul 27, 2023

jfberry commented Jul 27, 2023

Fabio1988 commented Aug 14, 2023

Mygod commented Aug 14, 2023

jfberry commented Aug 14, 2023

Mygod commented Aug 14, 2023

jfberry commented Aug 14, 2023

Mygod commented Aug 14, 2023

jfberry commented Aug 14, 2023

Mygod commented Aug 14, 2023

Mygod commented Aug 14, 2023

jfberry commented Aug 14, 2023

Mygod commented Jun 26, 2023 •

edited

Loading

ccev commented Jun 26, 2023 •

edited

Loading

Mygod Jun 29, 2023 •

edited

Loading