Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions nodejs/src/common/utils/db/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,11 @@ export function personInitialAndUTMProperties(properties: Properties): Propertie

// For the purposes of $initial properties, $os_name is treated as a fallback alias of $os, starting August 2024
// It's a special case due to _some_ SDKs using $os_name: https://github.com/PostHog/posthog-js-lite/issues/244
// normalizeOsAlias fills the event's own $os upstream, so the loop above already lifted $os into $set.
const osName = properties.$os_name
if (osName !== undefined && !skipServerHostOs) {
if (!('$os' in properties)) {
properties.$os = osName
}
if (!('$os' in $set)) {
$set.$os = osName
}
// Same null rule as above: a null $os_name must not claim $initial_os for good.
// Only reachable when $os is null, since the loop skips $initial_os for a null value; the
// non-null check on $os_name is the same first-touch rule as above.
if (osName !== null && !('$initial_os' in $set_once!)) {
$set_once!.$initial_os = osName
}
Expand Down
63 changes: 63 additions & 0 deletions nodejs/src/common/utils/event.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,69 @@ describe('normalizeEvent()', () => {
})
})

describe('$os_name alias', () => {
test.each([
{
name: 'fills a missing $os',
event: 'some_event',
properties: { $os_name: 'Windows' },
expected: 'Windows',
},
{
name: 'leaves an existing $os alone',
event: 'some_event',
properties: { $os: 'Mac OS X', $os_name: 'Windows' },
expected: 'Mac OS X',
},
{
name: 'carries a null $os_name through to $os',
event: 'some_event',
properties: { $os_name: null },
expected: null,
},
{
name: 'skips a $snapshot event, which skips person property lifting too',
event: '$snapshot',
properties: { $os_name: 'Windows' },
expected: undefined,
},
{
name: 'skips a $performance_event, which skips person property lifting too',
event: '$performance_event',
properties: { $os_name: 'Windows' },
expected: undefined,
},
])('$name', ({ event, properties, expected }) => {
Comment on lines +66 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test.each block covers $snapshot but not $performance_event, and does not cover a null $os_name. Both are cheap additions to the existing parameterized cases.

const normalized = normalizeEvent({ event, distinct_id: 'user1', properties } as any)
expect(normalized.properties!['$os']).toBe(expected)
})

it('gives the person $os and drops the $os_name alias', () => {
const normalized = normalizeEvent({
event: 'some_event',
distinct_id: 'user1',
properties: { $os_name: 'iOS' },
} as any)

expect(normalized.properties!['$set']).toEqual({ $os: 'iOS' })
expect(normalized.properties!['$set_once']).toEqual({ $initial_os: 'iOS' })
})

it('keeps a server host OS out of $os and $initial_os', () => {
const normalized = normalizeEvent({
event: 'some_event',
distinct_id: 'user1',
properties: { $is_server: true, $os_name: 'Linux' },
} as any)

expect(normalized.properties!['$os']).toBeUndefined()
// The host OS still reaches the person under the $os_name keys: personInitialAndUTMProperties
// skips only $os and $os_version for a server event, and $set_once makes $initial_os_name sticky.
expect(normalized.properties!['$set']).toEqual({ $os_name: 'Linux' })
expect(normalized.properties!['$set_once']).toEqual({ $initial_os_name: 'Linux' })
})
})

it('combines .properties $set and $set_once with top-level $set and $set_once', () => {
const event = {
event: 'some_event',
Expand Down
27 changes: 26 additions & 1 deletion nodejs/src/common/utils/event.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { PluginEvent } from '~/plugin-scaffold'
import { PluginEvent, Properties } from '~/plugin-scaffold'
import { ClickHouseEvent, PipelineEvent, PostIngestionEvent, RawClickHouseEvent } from '~/types'

import { personInitialAndUTMProperties, sanitizeString } from './db/utils'
Expand Down Expand Up @@ -136,6 +136,30 @@ export function sanitizeEvent<T extends PipelineEvent | PluginEvent>(event: T):
return event
}

/**
* Fills the event's own $os from the $os_name some SDKs report instead:
* https://github.com/PostHog/posthog-js-lite/issues/244.
*
* This describes the event, not the person. Call it after transformations: the user agent populator
* plugin skips an event that already carries $os, so filling $os earlier would stop it deriving
* $browser, $device, and the rest.
*
* Server events are skipped so a server's host OS does not land in the event's own $os. The person
* side is handled separately: personInitialAndUTMProperties skips lifting $os and $os_version and
* skips folding $os_name into the sticky $initial_os for $is_server events. The host OS still reaches
* the person under the raw $os_name and $initial_os_name keys.
*/
export function normalizeOsAlias(properties: Properties): void {
if (properties['$is_server'] === true) {
return
}

const osName = properties['$os_name']
if (osName !== undefined && !('$os' in properties)) {
properties['$os'] = osName
}
}

/**
* Full event normalization including person property mapping.
* This should only be called ONCE per event, after any transformations.
Expand All @@ -146,6 +170,7 @@ export function normalizeEvent<T extends PipelineEvent | PluginEvent>(event: T):
event = sanitizeEvent(event)

if (!['$snapshot', '$performance_event'].includes(event.event)) {
normalizeOsAlias(event.properties!)
event.properties = personInitialAndUTMProperties(event.properties!)
}

Expand Down
30 changes: 17 additions & 13 deletions nodejs/tests/utils/db/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,23 +101,28 @@ describe('personInitialAndUTMProperties()', () => {
$set: { $current_url: 'https://test.com' },
})
})
it('treats $os_name as fallback for $os', () => {
const propertiesOsNameOnly = { $os_name: 'Android' }
expect(personInitialAndUTMProperties(propertiesOsNameOnly)).toEqual({
$os: 'Android',
$os_name: 'Android',
$set_once: { $initial_os: 'Android' },
$set: { $os: 'Android' },
})
// Test that $os takes precedence, with $os_name preserved (although this should not happen in the wild)
const propertiesBothOsKeys = { $os: 'Windows', $os_name: 'Android' }
expect(personInitialAndUTMProperties(propertiesBothOsKeys)).toEqual({
it('keeps $os over $os_name and drops the alias from person properties', () => {
// normalizeOsAlias fills the event's own $os upstream, so this function always sees $os
// alongside $os_name; the divergent values here never occur in the wild.
const properties = { $os: 'Windows', $os_name: 'Android' }
expect(personInitialAndUTMProperties(properties)).toEqual({
$os: 'Windows',
$os_name: 'Android',
$set_once: { $initial_os: 'Windows' },
$set: { $os: 'Windows' },
})
})
it('falls back to $os_name for $initial_os when the event carries a null $os', () => {
// The never-write-null rule skips $initial_os for a null $os, leaving $os_name as the only
// first-touch value available.
const properties = { $os: null, $os_name: 'Android' }
expect(personInitialAndUTMProperties(properties)).toEqual({
$os: null,
$os_name: 'Android',
$set_once: { $initial_os: 'Android' },
$set: { $os: null },
})
})
it('does not lift device context from an $is_server event, but keeps campaign params', () => {
// A server SDK (is_server: true) stamps its host $os; it must not reach $set/$set_once or
// it permanently poisons $initial_os. utm is real attribution and still flows through.
Expand Down Expand Up @@ -156,7 +161,7 @@ describe('personInitialAndUTMProperties()', () => {
{ gclid: null, msclkid: null },
{},
],
['a null $os_name does not claim $initial_os', { $os_name: null }, { $os: null }, {}],
['a null $os_name does not claim $initial_os', { $os: null, $os_name: null }, { $os: null }, {}],
])('never writes null first-touch values: %s', (_desc, input, expectedSet, expectedSetOnce) => {
const result = personInitialAndUTMProperties({ ...input })
expect(result.$set).toEqual(expectedSet)
Expand All @@ -165,7 +170,6 @@ describe('personInitialAndUTMProperties()', () => {

it.each([
['client event maps $os', { $lib: 'web', $os: 'Linux' }, 'Linux'],
['client mobile via $os_name', { $lib: 'posthog-ios', $os_name: 'iOS' }, 'iOS'],
['no $is_server is treated as client', { $os: 'Linux' }, 'Linux'],
['$is_server false is treated as client', { $os: 'Linux', $is_server: false }, 'Linux'],
['$is_server true skips $os', { $os: 'Linux', $is_server: true }, undefined],
Expand Down
6 changes: 4 additions & 2 deletions rust/feature-flags/src/flags/flag_matching_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,10 @@ pub fn populate_missing_initial_properties(properties: &mut HashMap<String, Valu
///
/// Web SDKs (posthog-js) report the OS as `$os`; mobile SDKs report it as
/// `$os_name`. Ingestion normalizes `$os_name` -> `$os` at write time
/// (`personInitialAndUTMProperties` in nodejs/src/utils/db/utils.ts), but that
/// only covers newly-written rows — historical and un-normalized person rows can
/// (`normalizeOsAlias` in nodejs/src/common/utils/event.ts for the event's own
/// `$os`, `personInitialAndUTMProperties` in nodejs/src/common/utils/db/utils.ts
/// for the person row), but that only covers newly-written rows, and it excludes
/// `$is_server` events; historical and un-normalized person rows can
/// carry only one of the two keys. Flag matching does exact-key lookups, so
/// without this a mobile person whose row has only `$os_name` never matches an
/// `$os` filter (and vice versa). The mirror is bidirectional so either filter
Expand Down
Loading