add geoip processor #365
add geoip processor #365
Conversation
|
Hey @DpoBoceka, this looks good but I would rename the fields to be |
|
@Jeffail, Is there anything else I should fix? |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return res, nil |
There was a problem hiding this comment.
Returning a *City type might cause issues downstream as it's a struct. If you don't want to map the entire struct into map[string]interface{} then I'll forgive the lazier approach of json.Marshal(res) into raw bytes and use part.Set. Considering we're doing a DB call here I don't think the performance impact is something to worry about.
Same goes for asnReader.
| // GeoIP is a processor that looks up IP addresses. | ||
| type GeoIP struct { | ||
| parts []int | ||
| handler *geoip2.Reader |
There was a problem hiding this comment.
If you change this for a interface with the methods City and ASN then we can use a mock type for unit tests.
There was a problem hiding this comment.
I don't exactly understand, could you clarify a couple of things for me, please?
I could use an interface for replacing current GeoIP.reader an mock it but in order for that test/code to work I would be forced to explicitly declare processor.Geoip, not by calling NewGeoIP, because this would fail https://github.com/Jeffail/benthos/pull/365/files#diff-a4df8555a7687abaed0682a67d09ea3dR114
Is it ok to omit test for NewGeoIP or there is another way to refactor that code?
There was a problem hiding this comment.
Yeah that's fine, I'm mostly interested in testing ProcessMessage so I'm fine with declaring the struct instead of using NewGeoIP.
|
Hey @DpoBoceka, left some comments. Does a unit test here make any sense? Would be good to get some coverage here even if it's wrapping a mock implementation. I'm happy to do the docs when I have time. |
|
Hey @DpoBoceka, I think it makes sense to rework this into a bloblang method if you're open to that as it's much more flexible. I can take over if you don't have time. |
|
Hey @Jeffail, that would be just great. If you don't mind implementing that yourself I'd be delightful as I need to invest some time in figuring out the bloblang internals( I didn't use benthos for quite a while now, hence absence of my activity) |
|
Merged from #1068 |
hey, look what I've done.
Closes #277
Not sure If I handled closing handler to db right.
Have a look at it, please, and I'll add some tests if everything in order.