promotedai / ios-metrics-sdk

iOS client library for Promoted.ai metrics tracking.
MIT License
7 stars 1 forks source link

Allow clients to set ancestor IDs #124

Closed yunapotamus closed 3 years ago

yunapotamus commented 3 years ago

Background

Certain customers have internal tracking/logging already set up for users, sessions, and views. When we want to join the events that our library generates against the IDs that customers' internal systems generate, we need to be able to set the customers' IDs in the logUserID, sessionID, or viewID fields of MetricsLogger.

This came up in a situation where we are only logging impressions. These impression events need (at minimum) logUserID set in order to join them on the server side. Since we're not logging or generating user IDs, we need to set logUserID on those impressions to some value that the app generates.

In above situation, we won't expect requestID nor insertionID on impression events, and instead join using logUserID.

Proposal

Add the ability to set ancestor IDs in MetricsLogger for logUserID, sessionID, and viewID. Also expose these hooks to react-native-metrics. The client behavior will be as follows.

  1. If you set a custom ancestor ID, then MetricsLogger will use that custom ID going forward.
  2. If you change that custom ID, then we will use that new ID going forward.
  3. If you log an event that overrides that custom ID (say startSessionAndLogUser(userID:)), then we stop using the custom ID you set, and use our own autogenerated ID.
  4. If you never set a custom ID, then the library behaves exactly as before.

Pseudocode

/// Public-facing API for logUserID. You can set it via `logger.logUserID = "foo"`.
public var logUserID: String? {
  get { externalLogUserID ?? logUserIDProducer.currentValue }
  set { externalLogUserID = newValue }
}
/// Internal producer of UUIDs for logUserID.
private let logUserIDProducer: IDProducer
/// Overridden value for logUserID.
private let externalLogUserID: String?

An alternative would be to make this change on the IDProducer class.

final class IDProducer {

  // This becomes readwrite (it was readonly before).
  // Calls to getSessionInfo() for pending ancestor ID goes here.
  var currentValue: String? {
    didSet {
      // If client sets this to a custom value, then include
      // this ID in ancestor IDs going forward.
      valueHasBeenSet = true
    }
  }

  // Calls from MetricsLogger to populate ancestor IDs in
  // logged events go here.
  var currentValueOrPendingValue: String? {
    valueHasBeenSet ? currentValue : initialValue
  }

  // Internal state that allows us to hold on to pending 
  // ancestor IDs. We only return ancestor ID if this is true.
  private var valueHasBeenSet: Bool = false

  func nextValue() -> String {
    if !valueHasBeenSet {
      valueHasBeenSet = true
      currentValue = initialValue
      return currentValue
    }
    currentValue = nextValueProducer()
    return currentValue
  }
}

Calls from getSessionInfo() will read idProducer.currentOrPendingValue, which will return pending initial values even if those ancestor values haven't been explicitly set.

Calls from the logging methods on MetricsLogger when populating ancestor IDs will read idProducer.currentValue. This property will return EITHER the autogenerated value OR a custom value IF THE VALUE HAS BEEN SET. Otherwise, if no value is set, this property returns nil and the ancestor ID is omitted on the logged event.

Caveat

This approach precludes the use of custom IDs as pending ancestor IDs. That is, you can't set a custom ID and have it behave as a pending initial ID. This seems like an acceptable tradeoff as the setting of such a custom ID is a signal that the library should use that ID immediately going forward.

Usage

In the code where users sign in and their obfuscated user ID is available, we add the line:

PromotedMetrics.setLogUserID(user.id)

Then in all events logged from that point forward, the userInfo message will contain user.id in the logUserId field.

If a client app does not log any ancestor events with the library (user/session or view), and also does not pass through insertionID to its content, then it must set at least one ancestor ID prior to logging Action or Impression events. Ideally, this would be logUserID.

Sign off

Work begins when sign-off is received from all of the following:

yunapotamus commented 3 years ago

@danbosnichill @jinyius Please review at your convenience. @dtdennis28 Heads up of an upcoming change to the way IDs behave on the iOS library.

jinyius commented 3 years ago

just for completeness, we should explicitly call out that request id and insertion id for impression in these highly decoupled logging contexts aren't necessary since those will be usually handled by the platform's server (rpc) context.

yunapotamus commented 3 years ago

just for completeness, we should explicitly call out that request id and insertion id for impression in these highly decoupled logging contexts aren't necessary since those will be usually handled by the platform's server (rpc) context.

Done.

prm-dan commented 3 years ago

we won't expect requestID nor insertionID on impression events, and instead join using logUserID.

As of right now, yea. Eventually, we'll probably get insertionID too.

I'm not sure I understand the alternative proposal and how nextValue would work (reseting externalValue). The primary pseudo code looks good.

yunapotamus commented 3 years ago

we won't expect requestID nor insertionID on impression events, and instead join using logUserID.

As of right now, yea. Eventually, we'll probably get insertionID too.

Thanks for pointing that out. Since insertionID doesn't behave like the other ancestor IDs, that won't affect this proposal.

I'm not sure I understand the alternative proposal and how nextValue would work (reseting externalValue). The primary pseudo code looks good.

I updated the example and clarified the behavior.

Thanks for your reviews! I'll take this as sign-off for this proposal.

jinyius commented 3 years ago

@danbosnichill moving the discussion of luid and uid being set externally here per https://github.com/promotedai/ios-metrics-sdk/issues/126#issuecomment-854891336.

should we allow for setting both uid and luid externally? iiuc, right now, setting uid externally would generate a uuid local to the ios sdk.

yunapotamus commented 3 years ago

We discussed usage patterns over VC. This issue/PR brings up a larger theoretical question of how to handle these scenarios:

  1. The user wants the Promoted library to handle ancestor ID generation and log ancestor events, vs
  2. The user wants to handle ancestor ID generation and ancestor event logging, and only wants to push those ancestor IDs into the Promoted library.
  3. Any other usage pattern on the spectrum between 1 and 2. The user may want to log events both with Promoted and externally, set ancestor IDs for some events but not others, etc.

These two requirements potentially complicate the library, as these use cases could pull the API in different directions. #128 is an example of a potential mis-use of the API by supporting both use cases.

After discussion, we decided that we will only support the extremes of the spectrum. That is, either the user only does 1 or only does 2. Any mixing and matching will not be supported. Reasons for this:

  1. It can greatly complicate the API.
  2. It's still pretty early to tell which use case will be more common.
  3. Once we support something in the API, it will be more difficult to stop supporting it in the future.

As for the specific question of allowing external setting of userID, I won't include it in the first PR to be linked to this issue, but we can continue the discussion for a subsequent PR.