Analytics changes with tests. #1730
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor: Remove stale comments and TODOs from Windows Analytics Cleans up src/analytics_desktop.cc by: - Removing outdated TODO comments related to header verification and type definitions (Parameter/Item) that have since been resolved. - Deleting informational comments that were no longer accurate after refactoring (e.g., comments about placeholder types, forward declarations for removed types). - Removing an empty anonymous namespace that previously held an internal type alias. This commit improves the readability and maintainability of the Windows Analytics implementation by removing distracting or misleading commentary. The TODO regarding the precise Future creation mechanism in stubbed functions remains, as it requires further investigation of Firebase internal APIs.
Replaces placeholder comments with actual calls to LogError() and LogWarning() throughout the Windows Analytics implementation in src/analytics_desktop.cc. Key changes: - Added #include "firebase/log.h". - Updated error handling in LogEvent, ConvertParametersToGAParams, and SetUserProperty to use LogError() for invalid arguments or internal failures. - Updated warnings for unexpected data types or structures in event parameters to use LogWarning(). - Added LogWarning() calls to all stubbed functions (e.g., SetConsent, InitiateOnDeviceConversionMeasurement*, SetSessionTimeoutDuration) to inform developers that these operations are not supported and have no effect on the Windows platform. This change enhances the robustness and diagnosability of the Windows Analytics module by providing clear feedback through the Firebase logging system.
Based on explicit feedback, this commit fundamentally changes how
Parameter values of type Map and Vector are processed in the Windows
Analytics C++ SDK (in src/analytics_desktop.cc):
1. **Parameter values of type Vector (`param.value.is_vector()`):**
- These are now treated as UNSUPPORTED for top-level event parameters
on Desktop.
- An error is logged, and the parameter is skipped.
- The previous logic that interpreted a vector of maps as an item array
has been removed from ConvertParametersToGAParams.
2. **Parameter values of type Map (`param.value.is_map()`):**
- This case is now explicitly handled.
- The key-value pairs of the input map are transformed into a
GoogleAnalytics_ItemVector.
- Each entry (key, value_variant) in your map becomes a distinct
GoogleAnalytics_Item within this ItemVector.
- Each such GoogleAnalytics_Item stores the original map key under a
property named "name", and the original map value (which must be a
primitive type) under a typed property (e.g., "int_value",
"double_value", "string_value").
- This resulting ItemVector is then added to the event's parameters
using the original Parameter's name.
3. **Comments:**
- Code comments within ConvertParametersToGAParams have been updated
to reflect this new processing logic.
This change aligns the behavior with specific design requirements for how
map and vector type parameters should be translated to the underlying
Windows C API for Google Analytics.
This commit incorporates detailed review feedback to align the
Windows Analytics C++ implementation (src/analytics_desktop.cc)
with common Firebase C++ SDK patterns for managing Futures and
including headers.
Key changes:
1. **Future Management for Stubbed APIs:**
- Replaced the previous Future creation mechanism (MakeFuture) for
stubbed functions (GetAnalyticsInstanceId, GetSessionId, and their
LastResult counterparts) with the standard FutureData pattern.
- Added a static FutureData instance (g_future_data), initialized in
firebase::analytics::Initialize() and cleaned up in Terminate().
- Defined an internal AnalyticsFn enum for Future function tracking.
- Stubbed Future-returning methods now use g_future_data to create
and complete Futures.
- As per feedback, these Futures are completed with error code 0
and a null error message. A LogWarning is now used to inform
developers that the called API is not supported on Desktop.
- Added checks for g_future_data initialization in these methods.
2. **Include Path Updates:**
- Corrected #include paths for common Firebase headers (app.h,
variant.h, future.h, log.h) to use full module-prefixed paths
(e.g., "app/src/include/firebase/app.h") instead of direct
"firebase/" paths.
- Removed a stale include comment.
These changes improve the consistency of the Windows Analytics stubs
with the rest of the Firebase C++ SDK.
refactor: Address further review comments for Windows Analytics
This commit incorporates additional specific feedback on the Windows
Analytics C++ implementation (src/analytics_desktop.cc):
1. **Comment Cleanup:**
- Removed a commented-out unused constant and its description.
- Removed a non-functional comment next to a namespace declaration.
- Clarified and shortened the comment in the `is_vector` handling
block within `ConvertParametersToGAParams`.
2. **Use Common `AnalyticsFn` Enum:**
- Included `analytics/src/common/analytics_common.h`.
- Removed the local definition of `AnalyticsFn` enum, now relying on
the common definition (assumed to be in
`firebase::analytics::internal`).
3. **Corrected Map Parameter to `GoogleAnalytics_Item` Conversion:**
- Critically, fixed the logic in `ConvertParametersToGAParams` for when
a `Parameter::value` is a map.
- Previously, each key-value pair from the input map was incorrectly
creating a `GoogleAnalytics_Item` with fixed property names like
"name" and "int_value".
- The logic now correctly ensures that each key-value pair from your
input map becomes a direct property in the `GoogleAnalytics_Item`,
using the original map's key as the key for the property in the
`GoogleAnalytics_Item`. For example, an entry `{"user_key": 123}`
in the input map now results in a property `{"user_key": 123}`
within the `GoogleAnalytics_Item`.
This commit incorporates the latest round of specific feedback on the
Windows Analytics C++ implementation (src/analytics_desktop.cc):
1. **Comment Cleanup:**
- Removed a large commented-out block that previously contained a
local `AnalyticsFn` enum definition.
- Removed a comment in `Initialize()` related to marking the `app`
parameter as unused.
- Removed an outdated comment in `Initialize()` that described the
function as a placeholder.
- Removed a type comment from the `ConvertParametersToGAParams()`
function signature.
- Replaced a lengthy comment in `LogEvent()` regarding C API object
lifecycle with a more concise one.
2. **Refined Map Parameter Processing Logic:**
- In `ConvertParametersToGAParams`, when handling a `Parameter` whose
value is a map, the logic for creating `GoogleAnalytics_Item`
objects from the map's entries has been clarified.
- A local boolean flag (`successfully_set_property`) is used for each
map entry to track if a value was successfully set in the
corresponding `GoogleAnalytics_Item`.
- A `GoogleAnalytics_Item` is only added to the `GoogleAnalytics_ItemVector`
if a property was successfully set. Otherwise, the item is destroyed.
This prevents empty or partially formed items (e.g., from map entries
with unsupported value types) from being included in the ItemVector.
As per review feedback, this commit moves the Windows Analytics C++ implementation file from src/analytics_desktop.cc to analytics/src/analytics_desktop.cc. The content of the file remains the same as the previous version, incorporating all prior review feedback. I attempted to run the code formatter script after the move, but the script failed due to an internal error. Formatting was not applied.
a-maurice
reviewed
Jun 12, 2025
analytics/CMakeLists.txt
Outdated
| @@ -78,6 +78,11 @@ set(ios_SRCS | |||
| set(stub_SRCS | |||
| src/analytics_stub.cc) | |||
|
|
|||
| # Source files used by the stub implementation. | |||
There was a problem hiding this comment.
Should we delete the stub_SRCS above? And/or, update the comment? Do we want to do stuff here to strip out src/windows for Mac/Linux, or leave it up to the file?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Testing
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.