Skip to content

Analytics SetDefaultParameters #1262

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+146 −0
Draft

Analytics SetDefaultParameters #1262

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a047cec
Add SetDefaultParameters API to Firebase Analytics Unity (revised tests)
google-labs-jules[bot] Jun 3, 2025
59bdf9b
Fix: Ensure ClearDefaultEventParameters is wrapped for internal SDK use
google-labs-jules[bot] Jun 4, 2025
0ab4987
Refactor: Rename SetDefaultParameters to SetDefaultEventParameters
google-labs-jules[bot] Jun 5, 2025
f1c451b
Refactor: Use ModuleLogger in FirebaseAnalytics
google-labs-jules[bot] Jun 6, 2025
73efb08
Fix: Address review feedback for SetDefaultEventParameters
google-labs-jules[bot] Jun 7, 2025
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
24 changes: 24 additions & 0 deletions analytics/src/FirebaseAnalytics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,30 @@ public static void SetConsent(System.Collections.Generic.IDictionary< ConsentTyp
FirebaseAnalyticsInternal.SetConsentWithInts(consentSettingsMap);
}

/// @brief Sets default event parameters.
///
/// These parameters are logged with all events, in addition to the parameters passed to the
/// LogEvent() call. They are useful for logging common parameters with all events.
/// Default event parameters are overridden by event-specific parameters if the names are the
/// same. Default parameters are removed if the dictionary is null or empty.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's change it so that default parameters are only cleared if the dictionary is null. Individual parameters will be removed (by the underlying implementation) if the value of a given key is null.

///
/// @param[in] parameters The dictionary of parameters to set.
/// If null, clears all default parameters.
public static void SetDefaultParameters(
System.Collections.Generic.IDictionary<string, object> parameters) {
if (parameters == null || parameters.Count == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't do this if count = 0, only if it's null.

FirebaseAnalyticsInternal.ClearDefaultEventParameters();
} else {
StringList parameterNames = new StringList();
VariantList parameterValues = new VariantList();
foreach (var kvp in parameters) {
parameterNames.Add(kvp.Key);
parameterValues.Add(Firebase.Variant.FromObject(kvp.Value));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation iterates through the parameters dictionary and converts values using Firebase.Variant.FromObject(kvp.Value). If Firebase.Variant.FromObject encounters an unsupported type or fails to convert a value, it might throw an exception.

Consider how such exceptions should be handled:

  1. If an exception is thrown and not caught within this loop, the SetDefaultParameters call will terminate abruptly, potentially leaving the default parameters in an unchanged state (if the exception occurs before SetDefaultEventParametersHelper is called) or partially processed if this method were part of a larger try-catch. This could be disruptive to the calling application.
  2. A more robust approach might be to catch exceptions for individual parameter conversions, log a warning, and skip that specific parameter, allowing the rest of the valid parameters to be set.

Additionally, what is the intended behavior if the parameters dictionary is non-empty, but all parameters within it fail conversion and are skipped?

  • With the suggested try-catch below, if parameterNames and parameterValues end up empty, FirebaseAnalyticsInternal.SetDefaultEventParametersHelper would be called with empty lists. This would then call the native SetDefaultEventParameters with an empty map, which typically clears all existing default parameters.
  • Is clearing all defaults the desired outcome if the new set of parameters is entirely invalid? Or would it be preferable to preserve the existing default parameters in such a case (e.g., by not calling SetDefaultEventParametersHelper if parameterNames is empty after processing a non-empty input parameters dictionary)?

Could we enhance this part to gracefully handle potential conversion errors for individual parameters?

      foreach (var kvp in parameters) {
        try {
          // Attempt to convert the value to a Variant.
          Firebase.Variant variantValue = Firebase.Variant.FromObject(kvp.Value);
          // Note: If Variant.FromObject can return an invalid/null state for unsupported types
          // without throwing, an additional check for variantValue's validity might be needed here
          // (e.g., if (!variantValue.IsValid) { LogWarningAndSkip(); continue; } ).

          parameterNames.Add(kvp.Key);
          parameterValues.Add(variantValue);
        } catch (System.Exception e) { // Catch exceptions from FromObject (e.g., for unsupported types)
          // Log a warning and skip this parameter. Using UnityEngine.Debug.LogWarning for broad compatibility.
          UnityEngine.Debug.LogWarning(
              $"Firebase Analytics: Default parameter '{kvp.Key}' (value type: {kvp.Value?.GetType().FullName ?? "null"}) " +
              $"could not be converted and will be skipped. Error: {e.Message}");
        }
      }

FirebaseAnalyticsInternal.SetDefaultEventParametersHelper(parameterNames, parameterValues);
}
}

/// @brief Sets the duration of inactivity that terminates the current session.
///
/// @note The default value is 30 minutes.
Expand Down
25 changes: 25 additions & 0 deletions analytics/src/swig/analytics.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,24 @@ void SetConsentWithInts(const std::map<int, int>& settings) {
SetConsent(converted);
}

// Helper function to set default event parameters from vectors of strings and variants.
void SetDefaultEventParametersHelper(
const std::vector<std::string>& parameter_names,
const std::vector<firebase::Variant>& parameter_values) {
if (parameter_names.size() != parameter_values.size()) {
firebase::LogError(
"SetDefaultEventParametersHelper given different list sizes (%d, %d)",
parameter_names.size(), parameter_values.size());
return;
}

std::map<std::string, firebase::Variant> default_parameters;
for (size_t i = 0; i < parameter_names.size(); ++i) {
default_parameters[parameter_names[i]] = parameter_values[i];
}
SetDefaultEventParameters(default_parameters);
}

} // namespace analytics
} // namespace firebase

Expand All @@ -100,6 +118,9 @@ void SetConsentWithInts(const std::map<int, int>& settings) {
%ignore firebase::analytics::LogEvent(const char*, const Parameter*, size_t);
// Ignore SetConsent, in order to convert the types with our own function.
%ignore firebase::analytics::SetConsent;
// Ignore SetDefaultEventParameters and ClearDefaultEventParameters, as we handle them
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is outdated as we don't ignore ClearDefaultEventParameters.

// with a custom version or re-expose ClearDefaultEventParameters.
%ignore firebase::analytics::SetDefaultEventParameters;
// Ignore the Parameter class, as we don't want to expose that to C# at all.
%ignore firebase::analytics::Parameter;

Expand All @@ -121,5 +142,9 @@ namespace analytics {
void LogEvent(const char* name, std::vector<std::string> parameter_names,
std::vector<firebase::Variant> parameter_values);
void SetConsentWithInts(const std::map<int, int>& settings);
void SetDefaultEventParametersHelper(
const std::vector<std::string>& parameter_names,
const std::vector<firebase::Variant>& parameter_values);
void ClearDefaultEventParameters();
} // namespace analytics
} // namespace firebase
29 changes: 29 additions & 0 deletions analytics/testapp/Assets/Firebase/Sample/Analytics/UIHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,32 @@ public void AnalyticsSetConsent() {
});
}

public void TestSetDefaultParameters() {
DebugLog("Starting TestSetDefaultParameters...");

DebugLog("Setting single default string parameter: {'default_param_string', 'default_value_1'}");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>
{
{ "default_param_string", "default_value_1" }
});

DebugLog("Setting multiple default parameters: {'default_param_int', 123, 'default_param_double', 45.67, 'default_param_bool', true}");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>
{
{ "default_param_int", 123 },
{ "default_param_double", 45.67 },
{ "default_param_bool", true }
});

DebugLog("Clearing default parameters with null.");
FirebaseAnalytics.SetDefaultParameters(null);

DebugLog("Clearing default parameters with empty dictionary.");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>());

DebugLog("TestSetDefaultParameters completed.");
}

// Get the current app instance ID.
public Task<string> DisplayAnalyticsInstanceId() {
return FirebaseAnalytics.GetAnalyticsInstanceIdAsync().ContinueWithOnMainThread(task => {
Expand Down Expand Up @@ -257,6 +283,9 @@ void GUIDisplayControls() {
if (GUILayout.Button("Test SetConsent")) {
AnalyticsSetConsent();
}
if (GUILayout.Button("Test SetDefaultParameters")) {
TestSetDefaultParameters();
}
GUILayout.EndVertical();
GUILayout.EndScrollView();
}
Expand Down
29 changes: 29 additions & 0 deletions analytics/testapp/Assets/Firebase/Sample/Analytics/UIHandlerAutomated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public override void Start() {
TestAnalyticsSetConsentDoesNotThrow,
TestInstanceIdChangeAfterReset,
TestResetAnalyticsData,
TestSetDefaultParametersAutomated, // Added new test here
// Temporarily disabled until this test is deflaked. b/143603151
//TestCheckAndFixDependenciesInvalidOperation,
TestCheckAndFixDependenciesDoubleCall,
Expand Down Expand Up @@ -108,6 +109,34 @@ Task TestAnalyticsSetConsentDoesNotThrow() {
return true;
});
}

Task TestSetDefaultParametersAutomated() {
DebugLog("Automated Test: Starting TestSetDefaultParametersAutomated...");

DebugLog("Automated Test: Setting single default string parameter: {'auto_default_param_string', 'auto_value_1'}");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>
{
{ "auto_default_param_string", "auto_value_1" }
});

DebugLog("Automated Test: Setting multiple default parameters: {'auto_default_param_int', 789, 'auto_default_param_double', 12.34, 'auto_default_param_bool', false}");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>
{
{ "auto_default_param_int", 789 },
{ "auto_default_param_double", 12.34 },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should also test with a value of null to make sure that that's passed through correctly.

{ "auto_default_param_bool", false }
});

DebugLog("Automated Test: Clearing default parameters with null.");
FirebaseAnalytics.SetDefaultParameters(null);

DebugLog("Automated Test: Clearing default parameters with empty dictionary.");
FirebaseAnalytics.SetDefaultParameters(new Dictionary<string, object>());

DebugLog("Automated Test: TestSetDefaultParametersAutomated completed.");
return Task.FromResult(true); // Indicate successful completion
}

Task TestCheckAndFixDependenciesInvalidOperation() {
// Only run the test on Android, as CheckAndFixDependenciesAsync is short
// lived on other platforms, and thus could finish before the extra call.
Expand Down
Loading