Analytics SetDefaultParameters

[jonsimantov]

Description

Provide details of the change, and generalize the change in the PR title above.

Add SetDefaultParameters API to Firebase Analytics Unity SDK.

This commit introduces the FirebaseAnalytics.SetDefaultParameters() method to the Unity SDK, wrapping the underlying C++ firebase::analytics::SetDefaultEventParameters() and firebase::analytics::ClearDefaultEventParameters() functions.

---

Testing

Describe how you've tested these changes.

Calls to the new API have been integrated into the existing TestApp (UIHandler.cs) for manual verification and TestAppAutomated (UIHandlerAutomated.cs) for automated execution. These tests will exercise the API surface but not programmatically verify results from the Unity side.

---

Type of Change

Place an x the applicable box:

• Bug fix. Add the issue # below if applicable.
• New feature. A non-breaking change which adds functionality.
• Other, such as a build process or documentation change.

---

[gemini-code-assist]

Hello @jonsimantov, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here to provide a summary of this pull request. This PR introduces a new public API method, SetDefaultParameters, to the Firebase Analytics Unity SDK. This method allows developers to set parameters that will be automatically included with every subsequent event logged via LogEvent. It wraps the underlying C++ firebase::analytics::SetDefaultEventParameters and firebase::analytics::ClearDefaultEventParameters functions. The implementation includes handling for clearing default parameters when a null or empty dictionary is provided. The PR also includes updates to the test applications (manual and automated) to demonstrate and verify the basic functionality of this new API.

Highlights

• New API: SetDefaultParameters: Adds a new public static method FirebaseAnalytics.SetDefaultParameters to the Unity SDK. Allows setting default parameters that are automatically attached to all subsequent logged events.
• C++ Interop: Wraps the native C++ firebase::analytics::SetDefaultEventParameters and ClearDefaultEventParameters functions.
• Parameter Handling: Converts C# IDictionary<string, object> to C++ vectors of strings and variants using a new helper function in the SWIG interface.
• Test Coverage: Adds manual and automated tests in the sample applications (UIHandler.cs and UIHandlerAutomated.cs) to exercise the new API.

Changelog

Click here to see the changelog

• analytics/src/FirebaseAnalytics.cs
  • Added the public static method SetDefaultParameters (lines 260-273) to expose the functionality to Unity developers.
  • Implemented logic to convert the input dictionary to internal list types and call the appropriate internal C++ wrapper function (ClearDefaultEventParameters or SetDefaultEventParametersHelper).
• analytics/src/swig/analytics.i
  • Added a C++ helper function SetDefaultEventParametersHelper (lines 91-106) to facilitate converting vector inputs from C# (via SWIG) into a std::map for the native C++ SetDefaultEventParameters function.
  • Added %ignore directives for the original C++ SetDefaultEventParameters (lines 121-123) to prevent direct SWIG wrapping.
  • Added %include directives for the new SetDefaultEventParametersHelper and the existing ClearDefaultEventParameters (lines 145-148) to ensure they are wrapped by SWIG.
• analytics/testapp/Assets/Firebase/Sample/Analytics/UIHandler.cs
  • Added a new public method TestSetDefaultParameters (lines 165-189) to demonstrate the usage of the new API in the manual test app.
  • Added a button to the GUI (lines 286-288) to trigger the TestSetDefaultParameters method.
• analytics/testapp/Assets/Firebase/Sample/Analytics/UIHandlerAutomated.cs
  • Added a new automated test method TestSetDefaultParametersAutomated (lines 113-138) to programmatically test the new API.
  • Added TestSetDefaultParametersAutomated to the list of tests executed by the automated test runner (line 43).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Parameters set,
With every event they fly,
Analytics flow.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces the SetDefaultParameters API to the Firebase Analytics Unity SDK, providing a valuable feature for setting default parameters for all analytics events. The changes are well-structured, with corresponding C++ bindings and additions to both manual and automated test applications.

The implementation in C# and C++ appears correct for the happy path, and the SWIG interface changes are appropriate. The tests effectively exercise the new API surface.

One area for consideration is the robustness of the C# SetDefaultParameters method when encountering parameter values of unsupported types.

Summary of Findings

• Robustness to Invalid Parameter Types: The C# SetDefaultParameters method converts object values to Firebase.Variant. If Firebase.Variant.FromObject throws an exception for an unsupported type, the current implementation might lead to an unhandled exception. It's recommended to add error handling to skip problematic parameters and log a warning, ensuring the method behaves more gracefully with diverse inputs. The behavior when all provided parameters are invalid also warrants clarification (e.g., whether to clear existing defaults or preserve them).
• Code Quality and Testing: The overall code quality is good, with clear implementation and appropriate C++/SWIG bindings. The addition of both manual and automated tests is commendable and helps ensure the API surface is exercised.

Merge Readiness

The pull request introduces a useful feature and is generally well-implemented. However, there is a medium-severity concern regarding error handling for unsupported parameter types in FirebaseAnalytics.cs (detailed in the review comment).

I recommend addressing or clarifying the behavior for this scenario before merging to enhance the robustness of the new API. As I am an AI assistant, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after addressing the feedback. With the suggested improvement, the PR would be in a strong position for merging.

[gemini-code-assist]

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}");
        }
      }

[jonsimantov]

Let's hard code the tag instead of using typeof().ToString.

[jonsimantov]

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.

[jonsimantov]

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

[jonsimantov]

I don't think we need this originalCount logic any more if we change it so an empty dictionary is just passed through verbatim.

[jonsimantov]

Wouldn't it be cleaner to do the conversion first, and then add the name and value only once the conversion succeeds? This will prevent us from having to remove the value in the exception handler below.

[jonsimantov]

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

[jonsimantov]

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

[jonsimantov]

Also can we just confirm that Firebase.Variant.FromObject supports null properly, and will output a wrapped Variant::Null()?