Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change api #1133

Merged
merged 6 commits into from
Dec 17, 2024
Merged

change api #1133

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
590f416
change api
SabrinaTardio Dec 16, 2024
511f3a8
fix linting
SabrinaTardio Dec 17, 2024
f303cfa
remove commented out code
SabrinaTardio Dec 17, 2024
e5c0ba3
update some comments
SabrinaTardio Dec 17, 2024
9aa81e8
address comments
SabrinaTardio Dec 17, 2024
6b67ffb
fix linting
SabrinaTardio Dec 17, 2024
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
97 changes: 66 additions & 31 deletions Sources/PixelExperimentKit/PixelExperimentKit.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ extension PixelKit {
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)
}

/// Fires a pixel for a specific action in an experiment, based on conversion window and value thresholds (if value is a number).
/// Fires a pixel for a specific action in an experiment, based on conversion window and value.
/// - Parameters:
/// - subfeatureID: Identifier for the subfeature associated with the experiment.
/// - metric: The name of the metric being tracked (e.g., "searches").
Expand All @@ -82,7 +82,7 @@ extension PixelKit {
/// This function:
/// 1. Validates if the experiment is active.
/// 2. Ensures the user is within the specified conversion window.
/// 3. Tracks actions performed and sends the pixel once the target value is reached (if applicable).
/// 3. Sends the pixel if not sent before (unique by name and parameter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not be storing the pixel that was sent in this method so that we can ensure pixels are unique? (i.e not sent again if they were sent before). When the value being sent is unchanged, this method fires the same pixel on subsequent calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind the above, missed the uniqueness being handled in pixelkit

public static func fireExperimentPixel(for subfeatureID: SubfeatureID,
metric: String,
conversionWindowDays: ConversionWindow,
Expand All @@ -94,11 +94,43 @@ extension PixelKit {
}
guard let experimentData = featureFlagger.getAllActiveExperiments()[subfeatureID] else { return }

// Define event
let event = event(for: subfeatureID, experimentData: experimentData, conversionWindowDays: conversionWindowDays, metric: metric, value: value)

// Send unique by name and parameter pixel if within conversion window
let isInWindow = isUserInConversionWindow(conversionWindowDays, enrollmentDate: experimentData.enrollmentDate)
if isInWindow {
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: We could avoid the creation of the event and return early by moving the conversion window check up:

Suggested change
// Define event
let event = event(for: subfeatureID, experimentData: experimentData, conversionWindowDays: conversionWindowDays, metric: metric, value: value)
// Send unique by name and parameter pixel if within conversion window
let isInWindow = isUserInConversionWindow(conversionWindowDays, enrollmentDate: experimentData.enrollmentDate)
if isInWindow {
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)
}
// Check if within conversion window
guard isUserInConversionWindow(conversionWindowDays, enrollmentDate: experimentData.enrollmentDate) else { return }
// Define event
let event = event(for: subfeatureID, experimentData: experimentData, conversionWindowDays: conversionWindowDays, metric: metric, value: value)
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)

}

/// Fires a pixel for a specific action in an experiment, based on conversion window and value thresholds.
/// - Parameters:
/// - subfeatureID: Identifier for the subfeature associated with the experiment.
/// - metric: The name of the metric being tracked (e.g., "searches").
/// - conversionWindowDays: The range of days after enrollment during which the action is valid.
/// - numberOfCalls: target number of actions required to fire the pixel.
///
/// This function:
/// 1. Validates if the experiment is active.
/// 2. Ensures the user is within the specified conversion window.
/// 3. Tracks actions performed and sends the pixel once the target value is reached (if applicable).
public static func fireExperimentPixelWhenReachingNumberOfCalls(for subfeatureID: SubfeatureID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have any better name suggestion do let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could include "threshold" in the name, e.g:

fireExperimentPixelOnValueThreshold or fireExperimentPixelfThresholdReached

metric: String,
conversionWindowDays: ConversionWindow,
numberOfCalls: Int) {
// Check is active experiment for user
guard let featureFlagger = ExperimentConfig.featureFlagger else {
assertionFailure("PixelKit is not configured for experiments")
return
}
guard let experimentData = featureFlagger.getAllActiveExperiments()[subfeatureID] else { return }

fireExperimentPixelForActiveExperiment(subfeatureID,
experimentData: experimentData,
metric: metric,
conversionWindowDays: conversionWindowDays,
value: value)
numberOfCalls: numberOfCalls)
}

/// Fires search-related experiment pixels for all active experiments.
Expand Down Expand Up @@ -172,7 +204,7 @@ extension PixelKit {
experimentData: experimentData,
metric: metric,
conversionWindowDays: range,
value: "\(value)"
numberOfCalls: value
)
}
}
Expand All @@ -182,39 +214,24 @@ extension PixelKit {
experimentData: ExperimentData,
metric: String,
conversionWindowDays: ConversionWindow,
value: String) {
numberOfCalls: Int) {
// Set parameters, event name, store key
let eventName = "\(Constants.metricsEventPrefix)_\(subfeatureID)_\(experimentData.cohortID)"
let conversionWindowValue = (conversionWindowDays.lowerBound != conversionWindowDays.upperBound) ?
"\(conversionWindowDays.lowerBound)-\(conversionWindowDays.upperBound)" :
"\(conversionWindowDays.lowerBound)"
let parameters: [String: String] = [
Constants.metricKey: metric,
Constants.conversionWindowDaysKey: conversionWindowValue,
Constants.valueKey: value,
Constants.enrollmentDateKey: experimentData.enrollmentDate.toYYYYMMDDInET()
]
let event = ExperimentEvent(name: eventName, parameters: parameters)
let eventStoreKey = "\(eventName)_\(parameters.toString())"
let event = event(for: subfeatureID, experimentData: experimentData, conversionWindowDays: conversionWindowDays, metric: metric, value: String(numberOfCalls))
let parameters = parameters(metric: metric, conversionWindowDays: conversionWindowDays, value: String(numberOfCalls), experimentData: experimentData)
let eventStoreKey = "\(event.name)_\(parameters.toString())"

// Determine if the user is within the conversion window
let isInWindow = isUserInConversionWindow(conversionWindowDays, enrollmentDate: experimentData.enrollmentDate)

// Check if value is a number
if let numberOfAction = NumberOfActions(value), numberOfAction > 1 {
// Increment or remove based on conversion window status
let shouldSendPixel = ExperimentConfig.eventTracker.incrementAndCheckThreshold(
forKey: eventStoreKey,
threshold: numberOfAction,
isInWindow: isInWindow
)
// Increment or remove based on conversion window status
let shouldSendPixel = ExperimentConfig.eventTracker.incrementAndCheckThreshold(
forKey: eventStoreKey,
threshold: numberOfCalls,
isInWindow: isInWindow
)

// Send the pixel only if conditions are met
if shouldSendPixel {
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)
}
} else if isInWindow {
// If value is not a number, send the pixel only if within the window
// Send the pixel only if conditions are met
if shouldSendPixel {
ExperimentConfig.fireFunction(event, .uniqueByNameAndParameters, false)
}
}
Expand All @@ -233,6 +250,24 @@ extension PixelKit {
return currentDate >= calendar.startOfDay(for: startOfWindow) &&
currentDate <= calendar.startOfDay(for: endOfWindow)
}

private static func event(for subfeatureID: SubfeatureID, experimentData: ExperimentData, conversionWindowDays: ConversionWindow, metric: String, value: String) -> ExperimentEvent{
let eventName = "\(Constants.metricsEventPrefix)_\(subfeatureID)_\(experimentData.cohortID)"
let parameters = parameters(metric: metric, conversionWindowDays: conversionWindowDays, value: value, experimentData: experimentData)
return ExperimentEvent(name: eventName, parameters: parameters)
}

private static func parameters(metric: String, conversionWindowDays: ConversionWindow, value: String, experimentData: ExperimentData) -> [String: String] {
let conversionWindowValue = (conversionWindowDays.lowerBound != conversionWindowDays.upperBound) ?
"\(conversionWindowDays.lowerBound)-\(conversionWindowDays.upperBound)" :
"\(conversionWindowDays.lowerBound)"
return [
Constants.metricKey: metric,
Constants.conversionWindowDaysKey: conversionWindowValue,
Constants.valueKey: value,
Constants.enrollmentDateKey: experimentData.enrollmentDate.toYYYYMMDDInET()
]
}
}

extension Date {
Expand Down
Loading
Loading