Refactor the initial raw decoding #168
Conversation
comstud
commented
Oct 21, 2023
comstud
commented
- clean up the Raw HTTP route, move code to raw_decoder.
- move device location tracking to its own package/struct
- create a map of method -> decode function, minimum level required
- we don't need level 30 for everything
- map will be more efficient than big switch/case
- decode functions take in the proto and its metadata like device uuid, account, etc. This will allow us to pass information deeper for things like shiny stats which will require tracking accounts.
|
Untested yet, so marked as draft. Early feedback is welcome, though. |
| } | ||
| } | ||
|
|
||
| func (dec *rawDecoder) Decode(ctx context.Context, protoData *ProtoData, decodeFn func(context.Context, *Proto)) { |
There was a problem hiding this comment.
I intend to move the rest of the raw decoding out of 'main' and into this package, but for now this just takes a pointer to main's 'decode' function, shared between the http and grpc apis. This MR already does enough. I'll save more code shuffling for a follow-up.
|
I am running a version of this that is simply rebased on top of the the 'stats collector' PR that I've also made. If any of this is approved, I would suggest we take a look at the stats collector PR first and land that. Then I can push up the rebased version of this. And I have the next commit ready that moves the rest of decoding out of main and into the raw_decoder. But it is rebased on top of the rebased version of this... So I'm not going to PR that yet. But if interested in seeing where this is going, this is the commit: comstud@11866f0 |
| ) | ||
|
|
||
| type RawDecoder interface { | ||
| GetProtoDataFromHTTP(http.Header, []byte) (*ProtoData, error) |
There was a problem hiding this comment.
Do alternative ways of receiving data make sense to put in the same interface?
Surely we just get the data into a standard for (ProtoData I guess) and then pass on to a common processor?
There was a problem hiding this comment.
Yeah, this doesn't feel right to me, either. Not sure what I was smoking that night. Starting to refresh myself with what I did here.. since this was 1.5mo ago. :) I've been running with this patch this whole time, though. I'll fix this.
| } | ||
|
|
||
| func (pd ProtoData) Lat() float64 { | ||
| l := len(pd.Protos) |
There was a problem hiding this comment.
I've not followed exactly when this is called - for the standard MITM send, the lat/lon at the top of the body indicates where the calls were made from (and where processing rules are applied against); individual lat/lon on items aren't really used. I know this is what pogodroid does, but this is the exception I think.
There was a problem hiding this comment.
I also believe PD to be the exception, but since this object is the general object being built by the /raw decoder, it sort of needs to support it. Although, I don't remember how this is being used either. I will look it over.
| for _, proto := range protoData.Protos { | ||
| // provide independent cancellation contexts for each proto decode | ||
| ctx, cancel := context.WithTimeout(ctx, dec.decodeTimeout) | ||
| decodeFn(ctx, &proto) |
There was a problem hiding this comment.
We used to call each decoder in their own go-routine; however this caused too much tripping up and waiting on the striped mutex - since you might have responses that affect the same objects (eg during questing etc).
You also see some MITM include the gmo and the encounters in the same raw (even though this can't be how they were executed against the game).
Having independent cancellation is fine; originally intention was to be quite strict to not end in a backlog situation (which RDM is very common for when overloaded) - where you never catch up because you have so many concurrent decodes happening.
There was a problem hiding this comment.
This is actually copy/paste from existing code. But It looks like we don't need to make a new context for every single call here.
There was a problem hiding this comment.
(I'll reply to the other comments when I can )
|
I'm in favour of this refactor (and breaking out the decoders) - the split between the decode module and main has always been uncomfortable - it gradually evolved from golbat the prototype into golbat the production code. |
* clean up the Raw HTTP route, move code to raw_decoder.
* move device location tracking to its own package/struct
* create a map of method -> decode function, minimum level required
* we don't need level 30 for everything
* map will be more efficient than big switch/case
* decode functions take in the proto and its metadata like
device uuid, account, etc. This will allow us to pass
information deeper for things like shiny stats which will
require tracking accounts.
comstud
force-pushed
the
raw-decoding
branch
from
2a4b37a
to
8807961
Compare
