feat(livestream): Return geo data in the livestream service#44720
feat(livestream): Return geo data in the livestream service#44720jordanm-posthog wants to merge 2 commits intomasterfrom
Conversation
Greptile SummaryAdds geographic data (latitude, longitude, country code) to livestream event responses through a new |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return GeoResult{ | ||
| Latitude: &record.Location.Latitude, | ||
| Longitude: &record.Location.Longitude, | ||
| CountryCode: &record.Country.ISOCode, | ||
| }, nil |
There was a problem hiding this comment.
The code always returns a pointer to CountryCode even when the field is empty. MaxMind may return an empty string for iso_code when country data is unavailable. To maintain consistency with the nil-pointer pattern used for missing data, check if ISOCode is empty before returning a pointer:
| return GeoResult{ | |
| Latitude: &record.Location.Latitude, | |
| Longitude: &record.Location.Longitude, | |
| CountryCode: &record.Country.ISOCode, | |
| }, nil | |
| result := GeoResult{ | |
| Latitude: &record.Location.Latitude, | |
| Longitude: &record.Location.Longitude, | |
| } | |
| if record.Country.ISOCode != "" { | |
| result.CountryCode = &record.Country.ISOCode | |
| } | |
| return result, nil |
Prompt To Fix With AI
This is a comment left during a code review.
Path: livestream/geo/geoip.go
Line: 64:68
Comment:
The code always returns a pointer to `CountryCode` even when the field is empty. MaxMind may return an empty string for `iso_code` when country data is unavailable. To maintain consistency with the nil-pointer pattern used for missing data, check if `ISOCode` is empty before returning a pointer:
```suggestion
result := GeoResult{
Latitude: &record.Location.Latitude,
Longitude: &record.Location.Longitude,
}
if record.Country.ISOCode != "" {
result.CountryCode = &record.Country.ISOCode
}
return result, nil
```
How can I resolve this? If you propose a fix, please make it concise.| return GeoResult{ | ||
| Latitude: &record.Location.Latitude, | ||
| Longitude: &record.Location.Longitude, | ||
| CountryCode: &record.Country.ISOCode, | ||
| }, nil |
There was a problem hiding this comment.
Similarly, consider checking if latitude/longitude values are zero (0, 0), which typically indicates missing or invalid geo coordinates. While less critical than empty country codes, returning nil for (0, 0) coordinates would prevent false positives:
| return GeoResult{ | |
| Latitude: &record.Location.Latitude, | |
| Longitude: &record.Location.Longitude, | |
| CountryCode: &record.Country.ISOCode, | |
| }, nil | |
| result := GeoResult{} | |
| if record.Location.Latitude != 0 || record.Location.Longitude != 0 { | |
| result.Latitude = &record.Location.Latitude | |
| result.Longitude = &record.Location.Longitude | |
| } | |
| if record.Country.ISOCode != "" { | |
| result.CountryCode = &record.Country.ISOCode | |
| } | |
| return result, nil |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: livestream/geo/geoip.go
Line: 64:68
Comment:
Similarly, consider checking if latitude/longitude values are zero (0, 0), which typically indicates missing or invalid geo coordinates. While less critical than empty country codes, returning nil for (0, 0) coordinates would prevent false positives:
```suggestion
result := GeoResult{}
if record.Location.Latitude != 0 || record.Location.Longitude != 0 {
result.Latitude = &record.Location.Latitude
result.Longitude = &record.Location.Longitude
}
if record.Country.ISOCode != "" {
result.CountryCode = &record.Country.ISOCode
}
return result, nil
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| func convertToResponsePostHogEvent(event PostHogEvent, teamId int) *ResponsePostHogEvent { | ||
| geo := &GeoData{ | ||
| Lat: event.Lat, | ||
| Lng: event.Lng, | ||
| CountryCode: event.CountryCode, | ||
| } |
There was a problem hiding this comment.
The geo field is always created with a GeoData struct even when all fields are nil. This adds unnecessary data to the JSON response. Consider returning nil when no geo data is available:
| func convertToResponsePostHogEvent(event PostHogEvent, teamId int) *ResponsePostHogEvent { | |
| geo := &GeoData{ | |
| Lat: event.Lat, | |
| Lng: event.Lng, | |
| CountryCode: event.CountryCode, | |
| } | |
| var geo *GeoData | |
| if event.Lat != nil || event.Lng != nil || event.CountryCode != nil { | |
| geo = &GeoData{ | |
| Lat: event.Lat, | |
| Lng: event.Lng, | |
| CountryCode: event.CountryCode, | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: livestream/events/filter.go
Line: 73:78
Comment:
The `geo` field is always created with a `GeoData` struct even when all fields are nil. This adds unnecessary data to the JSON response. Consider returning nil when no geo data is available:
```suggestion
var geo *GeoData
if event.Lat != nil || event.Lng != nil || event.CountryCode != nil {
geo = &GeoData{
Lat: event.Lat,
Lng: event.Lng,
CountryCode: event.CountryCode,
}
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @@ -122,8 +127,9 @@ func TestParse(t *testing.T) { | |||
| "message_count": 0., | |||
| "message_kind": "event", | |||
| }, | |||
| Lat: 10, | |||
| Lng: 20, | |||
| Lat: ptrFloat64(10), | |||
| Lng: ptrFloat64(20), | |||
| CountryCode: ptrString("US"), | |||
| }, got) | |||
| } | |||
There was a problem hiding this comment.
Test already expects CountryCode in the result but doesn't explicitly assert it was set. Add an explicit assertion to verify the country code is correctly parsed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: livestream/events/kafka_test.go
Line: 109:134
Comment:
Test already expects CountryCode in the result but doesn't explicitly assert it was set. Add an explicit assertion to verify the country code is correctly parsed.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @@ -64,8 +69,8 @@ func TestPostHogKafkaConsumer_Consume(t *testing.T) { | |||
| assert.Equal(t, "test-distinct-id", event.DistinctId) | |||
| assert.Equal(t, "test-event", event.Event) | |||
| assert.Equal(t, "test-token", event.Token) | |||
| assert.Equal(t, 37.7749, event.Lat) | |||
| assert.Equal(t, -122.4194, event.Lng) | |||
| assert.Equal(t, 37.7749, *event.Lat) | |||
| assert.Equal(t, -122.4194, *event.Lng) | |||
| case <-time.After(time.Second): | |||
| t.Fatal("Timed out waiting for message") | |||
| } | |||
There was a problem hiding this comment.
Test verifies lat/lng but doesn't check CountryCode. Add assertion for the country code field to ensure complete geo data validation:
| // Mock GeoLocator Lookup | |
| mockGeoLocator.On("Lookup", "192.0.2.1").Return(geo.GeoResult{ | |
| Latitude: ptrFloat64(37.7749), | |
| Longitude: ptrFloat64(-122.4194), | |
| CountryCode: ptrString("US"), | |
| }, nil) | |
| // Run Consume in a goroutine | |
| go consumer.Consume() | |
| // Wait for the message to be processed | |
| select { | |
| case event := <-outgoingChan: | |
| assert.Equal(t, "test-uuid", event.Uuid) | |
| assert.Equal(t, "test-distinct-id", event.DistinctId) | |
| assert.Equal(t, "test-event", event.Event) | |
| assert.Equal(t, "test-token", event.Token) | |
| assert.Equal(t, 37.7749, *event.Lat) | |
| assert.Equal(t, -122.4194, *event.Lng) | |
| assert.Equal(t, "US", *event.CountryCode) | |
| case <-time.After(time.Second): | |
| t.Fatal("Timed out waiting for message") | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: livestream/events/kafka_test.go
Line: 56:76
Comment:
Test verifies lat/lng but doesn't check CountryCode. Add assertion for the country code field to ensure complete geo data validation:
```suggestion
// Mock GeoLocator Lookup
mockGeoLocator.On("Lookup", "192.0.2.1").Return(geo.GeoResult{
Latitude: ptrFloat64(37.7749),
Longitude: ptrFloat64(-122.4194),
CountryCode: ptrString("US"),
}, nil)
// Run Consume in a goroutine
go consumer.Consume()
// Wait for the message to be processed
select {
case event := <-outgoingChan:
assert.Equal(t, "test-uuid", event.Uuid)
assert.Equal(t, "test-distinct-id", event.DistinctId)
assert.Equal(t, "test-event", event.Event)
assert.Equal(t, "test-token", event.Token)
assert.Equal(t, 37.7749, *event.Lat)
assert.Equal(t, -122.4194, *event.Lng)
assert.Equal(t, "US", *event.CountryCode)
case <-time.After(time.Second):
t.Fatal("Timed out waiting for message")
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: livestream/events/filter_test.go
Line: 70:88
Comment:
Test doesn't verify that geo data is properly included in the response. Add assertions for the `Geo` field:
```suggestion
func TestConvertToResponsePostHogEvent(t *testing.T) {
timestamp := "2023-01-01T00:00:00Z"
event := PostHogEvent{
Uuid: "123",
Timestamp: timestamp,
DistinctId: "user1",
Event: "pageview",
Properties: map[string]interface{}{"url": "https://example.com"},
Lat: ptrFloat64(40.7128),
Lng: ptrFloat64(-74.0060),
CountryCode: ptrString("US"),
}
result := convertToResponsePostHogEvent(event, 1)
assert.Equal(t, "123", result.Uuid)
assert.Equal(t, "2023-01-01T00:00:00Z", result.Timestamp)
assert.Equal(t, "user1", result.DistinctId)
assert.NotEmpty(t, result.PersonId)
assert.Equal(t, "pageview", result.Event)
assert.Equal(t, "https://example.com", result.Properties["url"])
assert.NotNil(t, result.Geo)
assert.Equal(t, 40.7128, *result.Geo.Lat)
assert.Equal(t, -74.0060, *result.Geo.Lng)
assert.Equal(t, "US", *result.Geo.CountryCode)
}
```
How can I resolve this? If you propose a fix, please make it concise. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="livestream/events/kafka_test.go">
<violation number="1" location="livestream/events/kafka_test.go:73">
P2: Add assertion to verify CountryCode field is populated correctly from the geo lookup result.</violation>
</file>
<file name="livestream/geo/geoip.go">
<violation number="1" location="livestream/geo/geoip.go:67">
P2: Consider checking if `ISOCode` is empty before returning a pointer. MaxMind may return an empty string when country data is unavailable. Returning a pointer to an empty string breaks the nil-pointer pattern used elsewhere for missing data.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| assert.Equal(t, 37.7749, event.Lat) | ||
| assert.Equal(t, -122.4194, event.Lng) | ||
| assert.Equal(t, 37.7749, *event.Lat) | ||
| assert.Equal(t, -122.4194, *event.Lng) |
There was a problem hiding this comment.
P2: Add assertion to verify CountryCode field is populated correctly from the geo lookup result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At livestream/events/kafka_test.go, line 73:
<comment>Add assertion to verify CountryCode field is populated correctly from the geo lookup result.</comment>
<file context>
@@ -64,8 +69,8 @@ func TestPostHogKafkaConsumer_Consume(t *testing.T) {
- assert.Equal(t, 37.7749, event.Lat)
- assert.Equal(t, -122.4194, event.Lng)
+ assert.Equal(t, 37.7749, *event.Lat)
+ assert.Equal(t, -122.4194, *event.Lng)
case <-time.After(time.Second):
t.Fatal("Timed out waiting for message")
</file context>
| assert.Equal(t, -122.4194, *event.Lng) | |
| assert.Equal(t, -122.4194, *event.Lng) | |
| assert.NotNil(t, event.CountryCode) | |
| assert.Equal(t, "US", *event.CountryCode) |
| CountryCode: &record.Country.ISOCode, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
P2: Consider checking if ISOCode is empty before returning a pointer. MaxMind may return an empty string when country data is unavailable. Returning a pointer to an empty string breaks the nil-pointer pattern used elsewhere for missing data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At livestream/geo/geoip.go, line 67:
<comment>Consider checking if `ISOCode` is empty before returning a pointer. MaxMind may return an empty string when country data is unavailable. Returning a pointer to an empty string breaks the nil-pointer pattern used elsewhere for missing data.</comment>
<file context>
@@ -26,22 +32,38 @@ func NewMaxMindGeoLocator(dbPath string) (*MaxMindLocator, error) {
+ return GeoResult{
+ Latitude: &record.Location.Latitude,
+ Longitude: &record.Location.Longitude,
+ CountryCode: &record.Country.ISOCode,
+ }, nil
}
</file context>
jordanm-posthog
changed the title
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| var v1 interface{} | ||
| if m, ok := v1.(easyjson.Unmarshaler); ok { | ||
| m.UnmarshalEasyJSON(in) | ||
| } else if m, ok := v1.(json.Unmarshaler); ok { | ||
| _ = m.UnmarshalJSON(in.Raw()) | ||
| } else { | ||
| v1 = in.Interface() | ||
| } |
There was a problem hiding this comment.
Critical bug in property unmarshaling logic. The variable v1 is declared as interface{} (nil) before type assertions, so the type checks will always fail and always execute the else branch. This breaks custom unmarshaling for properties that implement easyjson.Unmarshaler or json.Unmarshaler.
What breaks: Properties with custom unmarshaling logic will fail to unmarshal correctly, potentially causing data corruption or runtime panics.
Fix:
v1 := in.Interface()
if m, ok := v1.(easyjson.Unmarshaler); ok {
m.UnmarshalEasyJSON(in)
} else if m, ok := v1.(json.Unmarshaler); ok {
_ = m.UnmarshalJSON(in.Raw())
}
(out.Properties)[key] = v1The original pattern should be restored - assign v1 first, then perform type assertions on the actual value.
| var v1 interface{} | |
| if m, ok := v1.(easyjson.Unmarshaler); ok { | |
| m.UnmarshalEasyJSON(in) | |
| } else if m, ok := v1.(json.Unmarshaler); ok { | |
| _ = m.UnmarshalJSON(in.Raw()) | |
| } else { | |
| v1 = in.Interface() | |
| } | |
| v1 := in.Interface() | |
| if m, ok := v1.(easyjson.Unmarshaler); ok { | |
| m.UnmarshalEasyJSON(in) | |
| } else if m, ok := v1.(json.Unmarshaler); ok { | |
| _ = m.UnmarshalJSON(in.Raw()) | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Problem
The Livestream client currently doesn't expose a way to get geo data alongside the event data. Additionally, the geo data only included lat / lng.
Changes
GeoLocatorto return aGeoResultstruct instead of separate lat/lng values, making it easier to extend with additional geo fields in the futureFound()method.How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?