chore(livestream): Return Country Code from Livestream geo stream#45765
chore(livestream): Return Country Code from Livestream geo stream#45765jordanm-posthog merged 4 commits intomasterfrom
Conversation
lricoy
left a comment
lricoy
left a comment
There was a problem hiding this comment.
LGTM, but low context. Wait for Pawel if you're unsure about it
| } | ||
|
|
||
| subscription := events.Subscription{ | ||
| SubID: atomic.AddUint64(&subID, 1), |
There was a problem hiding this comment.
No, but it won't hurt :) I was playing around with running gofmt as a pre-commit hook, so this change will make it's way in eventually.
I'll give Pawel some time to review. I'm not too worried though, geo hasn't seen any traffic in the last week so it's pretty low stakes |
orian
left a comment
orian
left a comment
There was a problem hiding this comment.
The geo wasn't used for the last 1.5y, I'm not sure how fast it is, I was thinking about remove it but didn't have enough time. When I've joined it also had some bugs (e.g. it was global, without proper filtering per team/token).
| phEvent.Lat, phEvent.Lng, err = geolocator.Lookup(ipStr) | ||
| phEvent.Lat, phEvent.Lng, phEvent.CountryCode, err = geolocator.Lookup(ipStr) | ||
| if err != nil && err.Error() != "invalid IP address" { // An invalid IP address is not an error on our side | ||
| // TODO capture error to PostHog |
There was a problem hiding this comment.
this is terrible idea, some metric may be useful
There was a problem hiding this comment.
I imagine this is terrible because it's slow? If that's the case, maybe a custom unmarshaler would help.
There was a problem hiding this comment.
IMO this is potentially millions of errors with nothing very useful, and this is not a real error, just a regular flow.
We probably could log a line with failing IP, not sure how big would be a value out of it? I don't think this service is right place to validate the IP.
Also, it's probably me who wrote this comment :D
There was a problem hiding this comment.
Ohh, I see. I thought your comment was on getting the country code :) I don't think logging the IP would be much value since there isn't really any action we could take to resolve it. Your initial comment about a metric is sound though, it'd be nice to have so visibility into how often we can't find the location.
There was a problem hiding this comment.
If we want to drastically improve the performance of livestream, I would skip processing events for tokens that have no subscription. Sure, when someone opens liveview the number of open sessions would be empty, but probably 95% of events could be skipped.
Other, not that crazy idea: instead using tokens: use real team_id. The tokens map perfectly to team_ids and this map would be not that big.
There was a problem hiding this comment.
If we want to drastically improve the performance of livestream, I would skip processing events for tokens that have no subscription
I have a change for that locally already :) I just need to test a few things before I open a PR.
Other, not that crazy idea: instead using tokens: use real team_id. The tokens map perfectly to team_ids and this map would be not that big.
I can take a look into this as well
What are your thoughts on merging geo with the events stream in that case? I have a draft PR here to do that. I was a bit worried about speed initially, but maybe that isn't an issue if it's opt-in + we drop messages for slow clients |
3 participants
Problem
The Livestream geo feed only returns lat / lng but country code would be useful for the Web Analytics live dashboard.
Changes
Adds the IP's country code to
ResponseGeoEventHow did you test this code?
Updated tests, tested locally
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
no