promotedai / ios-metrics-sdk

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

Warn when logging events with no ancestor IDs set #126

Closed yunapotamus closed 2 years ago

yunapotamus commented 3 years ago

When logging Action or Impression events, if there are no ancestor IDs available, warn the client that they need at least one ancestor ID for the events to be useful. The events themselves can still be logged to server.

Pseudocode

func logImpression(insertionID: String?) {
  if logUserID == nil && viewID == nil && insertionID == nil {
    warning("Need at least one ancestor ID")
  }
  // Log the event
}

// Same check at start of logAction

Sign off

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

yunapotamus commented 3 years ago

Jin asks:

@danbosnichill, are you ok omitting userId from this check? if we include it, we'll have to prioritize doing uid based joining to get a consistent luid (can also extend to session and client hints) if we accept uid as a join key. basically, the inferred user stuff at the limit. i'm fine omitting for now to avoid signing us up to prioritize this work.

If userID is not nil, then logUserID won't be nil either, so adding userID to this check won't change its behavior.

jinyius commented 3 years ago

if uid was supplied from external means, why would luid not be nil either? are you assuming the mobile app would handle the user auth events, and therefore, be authoritative for luid and uid?

On Thu, Jun 3, 2021 at 3:29 PM Y Wang @.***> wrote:

Jin asks:

@danbosnichill https://github.com/danbosnichill, are you ok omitting userId from this check? if we include it, we'll have to prioritize doing uid based joining to get a consistent luid (can also extend to session and client hints) if we accept uid as a join key. basically, the inferred user stuff at the limit. i'm fine omitting for now to avoid signing us up to prioritize this work.

If userID is not nil, then logUserID won't be nil either, so adding userID to this check won't change its behavior.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/promotedai/ios-metrics-sdk/issues/126#issuecomment-854226944, or unsubscribe https://github.com/notifications/unsubscribe-auth/AS3KMVIH7EASMNV3WAFTYQDTQ764ZANCNFSM46BNP2UA .

yunapotamus commented 3 years ago

if uid was supplied from external means, why would luid not be nil either? are you assuming the mobile app would handle the user auth events, and therefore, be authoritative for luid and uid?

You can't supply just userID externally. Setting userID causes us to generate an internal logUserID.

You CAN supply just logUserID externally, in which case userID will be nil/unchanged. That's the functionality in #124.

jinyius commented 3 years ago

this might be good to bike shed on since you're looking for pseudo code impl in these issues afaiu. can uid and luid both be given from/set externally?

On Thu, Jun 3, 2021 at 3:35 PM Y Wang @.***> wrote:

if uid was supplied from external means, why would luid not be nil either? are you assuming the mobile app would handle the user auth events, and therefore, be authoritative for luid and uid?

You can't supply just userID externally. Setting userID causes us to generate an internal logUserID.

You CAN supply just logUserID externally, in which case userID will be nil. That's the functionality in #124 https://github.com/promotedai/ios-metrics-sdk/issues/124.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/promotedai/ios-metrics-sdk/issues/126#issuecomment-854229661, or unsubscribe https://github.com/notifications/unsubscribe-auth/AS3KMVKGP575OZSDLF2ZFPLTQ77SVANCNFSM46BNP2UA .

yunapotamus commented 3 years ago

They can both be set externally, but setting userID is a separate use case from setting logUserID.

jinyius commented 3 years ago

if both are set externally, why not keep both? @danbosnichill to add his thoughts on this detail as well.

yunapotamus commented 3 years ago

Because setting both userID and logUserID externally is not a use case that we plan to support in #124. If you set logUserID externally then you're logging user events in some other way, and you're generating your own obfuscated logUserID for us to use. In this case the Promoted client library doesn't handle the logging of userID.

If you propose to change the behavior as specified in #124, please comment in that issue instead.