amplitude / Amplitude-Node

Server-side Node.js SDK for Amplitude
MIT License
67 stars 20 forks source link

OS is always set to undefined #78

Closed tommedema closed 3 years ago

tommedema commented 3 years ago

Expected Behavior

When sending os_name amplitude should pickup the OS

Current Behavior

The OS is set to undefined

Possible Solution

Honor the os_name field

Steps to Reproduce

  1. send an event with this event object;
{
  user_id: '60245c08b45ec4007022ec13',
  device_id: 'c3dae446-7d15-44fb-9751-16e4b38df58b',
  event_type: 'ACTION_INSERT_TOPIC_THREAD_COMMENT',
  insert_id: 'bbe59f0d-2bc2-47a7-8543-684072e7785d',
  time: 1612996899714,
  session_id: '1612996076612',
  app_version: '@bubbles/web-ui@2.0.3',
  language: 'en-US',
  ip: '71.198.20.116',
  platform: 'Chrome@88.0.4324.146',
  os_name: 'Mac OS',
  os_version: '10.15.7',
  device_brand: undefined,
  device_model: undefined,
  event_properties: {
    viewId: 'e155c739-3f77-46f7-8123-2dd5dea599fc',
    originTopicId: 'e155c739-3f77-46f7-8123-2dd5dea599fc',
    originUserEmail: undefined,
    originUserName: 'Amplitude2',
    isUserAction: false,
    actionType: 'INSERT_TOPIC_THREAD_COMMENT',
    actionId: 'bbe59f0d-2bc2-47a7-8543-684072e7785d',
    isServerGenerated: undefined,
    userAgent: {
      ua: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.146 Safari/537.36',
      browser: { name: 'Chrome', version: '88.0.4324.146', major: '88' },
      engine: { name: 'Blink', version: '88.0.4324.146' },
      os: { name: 'Mac OS', version: '10.15.7' },
      device: { vendor: undefined, model: undefined, type: undefined },
      cpu: { architecture: undefined }
    },
    payload: {
      threadId: '01cdebcd-39b5-4714-9321-c5644da49f32',
      archived: false,
      topicId: 'e155c739-3f77-46f7-8123-2dd5dea599fc',
      participantEmails: [ [length]: 0 ],
      name: 'Amplitude2',
      id: '2c916ba6-ec74-4632-9a54-c5ca652102ec',
      body: 'test2',
      userId: '60245c08b45ec4007022ec13',
      timestamp: 1612996899918
    }
  },
  user_properties: {
    originUserEmail: undefined,
    originUserName: 'Amplitude2',
    userAgent: {
      ua: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.146 Safari/537.36',
      browser: { name: 'Chrome', version: '88.0.4324.146', major: '88' },
      engine: { name: 'Blink', version: '88.0.4324.146' },
      os: { name: 'Mac OS', version: '10.15.7' },
      device: { vendor: undefined, model: undefined, type: undefined },
      cpu: { architecture: undefined }
    }
  }
}
  1. note that the event OS is undefined in the amplitude log stream:

https://app.usebubbles.com/wSsTyZ7XMMan6v8LVPufRN/amplitude-os-undefined/

Environment

tommedema commented 3 years ago

It's also really weird how https://developers.amplitude.com/docs/http-api-v2 defines os_name as:

The name of the mobile operating system or browser that the user is using.

How are we supposed to distinguish between the browser on a mobile operating system then? And why is there no browser property?

As a workaround I was planning to set the operating system under the "platform" property, but for some reason this library always overwrites that to Node.js -- which is not the desired behavior because we're proxying user events, and we care about the user's platform/OS, not our server's.

kelvin-lu commented 3 years ago

@tommedema This might be an issue with how the HTTP API is spec'd: link:

These fields (platform, os_name, os_version, device_brand, device_manufacturer, device_model, and carrier) must all be updated together. Setting any of these fields will automatically reset all of the other property values to null if they are not also explicitly set for the same event.

A workaround while we figure out a better solution (either on the Data ingestion or the SDK side) might be unsetting the platform (event.platform = '')

tommedema commented 3 years ago

@kelvin-lu if we unset the platform, where do we set the operating system then? You'd think os_name, but that is reserved for the browser the user is using if they're not on mobile (which is really weird).

The name of the mobile operating system or browser that the user is using.

tommedema commented 3 years ago

@kelvin-lu this is our workaround for now, which doesn't seem great, but would appreciate your feedback:

    // Since Amplitude does not distinguish between operating systems and browsers,
    // We pass in the user agent details as event and user properties in addition to the below mixture.
    platform: `${originUserAgent.browser.name}@${originUserAgent.browser.major}`,
    // See https://developers.amplitude.com/docs/http-api-v2#footnotes
    // On why we must specify `unknown` instead of undefined here
    os_name: originUserAgent.os.name || 'unknown',
    os_version: originUserAgent.os.version || 'unknown',
    device_brand: originUserAgent.device.vendor || 'unknown',
    device_model: originUserAgent.device.model || 'unknown',
    device_manufacturer: 'unknown',
    carrier: 'unknown',
kelvin-lu commented 3 years ago

I think this makes a lot of sense. Apologies for the slow reply, relaying information from our Data Pipeline Team:

We previously mentioned that the location properties act as a group (i.e. they must all be updated together), and the same is true of the device properties: Device Family, Device Type, Carrier, Platform, and OS make up this group. Once again, this is so we don’t end up with inconsistent values, e.g. Device Type = Apple iPhone X but Platform = Android.

I don't think we'll take the direction of removing Platform off of the SDK so that the others are not reset, but we might make it easier to set the os, version, brand, model, etc. only once instead of on every event

tommedema commented 3 years ago

@kelvin-lu thanks.

When you say "Device Family, Device Type", are you referring to these props or are we missing something?

device_brand
device_model
device_manufacturer
kelvin-lu commented 3 years ago

Oh I pulled this verbatim off an internal guide, sorry!

Device Family and Device Type are the human-readable properties in the Amplitude UI that we generate out of device_model and device_manufacturer. There's nothing you need to do on your end besides making sure those two fields are populated.

kelvin-lu commented 3 years ago

@tommedema as another fix for this we're planning on removing the defaultNode.js platform from events in the Node SDK, so this does not cause unexpected behavior with other fields (os, device, etc.)

tommedema commented 3 years ago

That's works, as long as we can still set the platform directly.

ajhorst commented 3 years ago

fixed with v1.5.2, which is now available