Gainsight PX Device Mode new integration #1684
Conversation
gs-nwolfe
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
|
@gs-nwolfe No need to create a new destination |
src/configurations/destinations/gainsight_px_browser/db-config.json
Outdated
Show resolved
Hide resolved
PR feedback fixed problem with apiKey/productKey confusion
Placeholder for datacenter
@Gauravudia - Can you clarify what needs to be done here? I see the existing "GAINSIGHT_PX" entry in ./configurations/destinations/gainsight_px/db-config.json. How should we go about merging this one with my new destination? I don't want to break any of our existing customers that are already using the old cloud mode destination. |
We to have update the ./configurations/destinations/gainsight_px/db-config.json to add the support for device mode. |
WalkthroughThree new configuration files have been added for the Gainsight PX browser integration: Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR feedback - removed extra config
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/configurations/destinations/gainsight_px_browser/db-config.json (4)
1-3: Consider merging with existing Gainsight PX integrationThe PR objectives suggest updating the existing "GAINSIGHT_PX" destination to support both cloud and device modes, rather than creating a new "GAINSIGHT_PX_BROWSER" destination. This approach would align with the suggestion made by Gauravudia in the PR comments.
Consider discussing with the team whether it would be more appropriate to update the existing Gainsight PX integration in
./configurations/destinations/gainsight_px/db-config.jsonto support both cloud and device modes, similar to the Ninetailed destination mentioned in the PR comments. This could help streamline the integration process and avoid redundancy.
4-20: Configuration looks good, minor suggestion for clarityThe configuration in this section is well-structured and aligns with the PR objectives. It correctly specifies the supported source types, connection modes, and message types for the device mode integration.
For improved clarity, consider adding a brief comment explaining the purpose of the
transformAtV1andsaveDestinationResponsesettings. This would help other developers understand the significance of these configurations.
21-32: destConfig looks good, minor optimization suggestedThe
destConfigsection is well-structured and includes all necessary keys for both default and web-specific configurations.To avoid redundancy, consider removing
consentManagementfrom thewebarray since it's already included in thedefaultConfigarray. This would ensure that the key is not duplicated in the configuration.
1-35: Overall, excellent configuration with one architectural considerationThe
db-config.jsonfile is well-structured, comprehensive, and aligns closely with the PR objectives for the Gainsight PX browser integration. It correctly configures the necessary settings for device mode, including supported source types, connection modes, and message types.While the configuration itself is solid, the main point to consider is whether this should be a separate integration (GAINSIGHT_PX_BROWSER) or an update to the existing GAINSIGHT_PX integration to support both cloud and device modes. This decision could impact the overall architecture and maintainability of the Gainsight PX integration in RudderStack. Consider discussing this with the team to ensure the chosen approach aligns with the long-term vision for the product.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/configurations/destinations/gainsight_px_browser/db-config.json (1 hunks)
- src/configurations/destinations/gainsight_px_browser/schema.json (1 hunks)
- src/configurations/destinations/gainsight_px_browser/ui-config.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/configurations/destinations/gainsight_px_browser/schema.json
🧰 Additional context used
🔇 Additional comments (5)
src/configurations/destinations/gainsight_px_browser/db-config.json (1)
33-33: secretKeys configuration is appropriateThe
secretKeysarray correctly includes "productKey", which is appropriate for maintaining the security of sensitive information.This configuration aligns with best practices for handling API keys and other sensitive data.
src/configurations/destinations/gainsight_px_browser/ui-config.json (4)
3-51: Connection Settings section looks good.The Connection Settings section is well-structured with appropriate fields:
- The Product Key field has a suitable regex pattern for validation.
- The Data Center options cover the necessary choices (US, EU, US2).
- The Additional Configuration field provides flexibility for advanced users.
52-62: Native SDK section is appropriately configured.The Native SDK section contains a single checkbox for using device-mode to send events, which aligns with the PR objective of introducing device mode integration. Setting it to true by default is a good choice for encouraging the use of this new feature.
63-117: Client-side Events Filtering section is well-designed.The Client-side Events Filtering section provides a flexible and user-friendly configuration for event filtering:
- Users can choose between disabling filtering, allowlisting, or denylisting events.
- Dynamic forms for entering event names in both allowlist and denylist provide ease of use.
- The section note clearly states that this is applicable only for device-mode integrations.
1-231: Overall, the UI configuration is well-structured and aligns with PR objectives.The
ui-config.jsonfile provides a comprehensive and well-organized configuration for the Gainsight PX browser integration. It successfully addresses the PR objective of introducing a device mode integration with the following strengths:
- Clear separation of concerns with distinct sections for different aspects of the integration.
- Flexible options for connection settings, event filtering, and consent management.
- Good use of dynamic forms and select inputs to simplify user interaction.
- Appropriate default values and validation patterns where necessary.
The configuration strikes a good balance between simplicity for basic users and flexibility for advanced users. It should provide a solid foundation for implementing the new Gainsight PX browser integration.
@Gauravudia - Thanks for the help. I need additional assistance with the merge process. The ui config for the new device mode destination is different than the config for the old cloud mode destination. I don't understand how to have two different configs in the same directory. I don't see two blocks of config info in the ninetailed destination, am I missing something? I don't want to break any existing customers that are using the current web mode destination. |
I have raised the Integration config PR to add the support for device mode |
@shrouti1507 I have already raised the PR for that. |
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/configurations/destinations/gainsight_px_browser/db-config.json (1)
1-3: Merge with existing Gainsight PX destination instead of creating a new one.
Based on the PR discussion, this should be integrated into the existing Gainsight PX destination (GAINSIGHT_PX) rather than creating a new destination. This approach:
- Maintains a single destination supporting both cloud and device modes
- Follows the pattern used by other destinations (e.g., Ninetailed)
- Prevents redundancy and simplifies maintenance
The configuration should be moved to ./configurations/destinations/gainsight_px/db-config.json and merged with the existing cloud mode configuration.
src/configurations/destinations/gainsight_px_browser/ui-config.json (2)
1-42: Consider merging with existing Gainsight PX integration.
Based on the PR discussion, instead of creating a new destination named "GAINSIGHT_PX_BROWSER", this configuration should be integrated into the existing "GAINSIGHT_PX" destination at ./configurations/destinations/gainsight_px/. This approach would:
- Maintain a single source of truth for both cloud and device modes
- Follow the pattern used by other destinations like Ninetailed
- Align with the existing PR (feat: gainsight px device mode support #1741)
54-108: Update to more inclusive terminology.
While the UI shows "Allowlist" and "Denylist", the underlying values still use "whitelistedEvents" and "blacklistedEvents". Consider updating these to align with inclusive terminology:
- "value": "whitelistedEvents"
+ "value": "allowlistedEvents"- "value": "blacklistedEvents"
+ "value": "deniedEvents"📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/configurations/destinations/gainsight_px_browser/db-config.json (1 hunks)
- src/configurations/destinations/gainsight_px_browser/ui-config.json (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/configurations/destinations/gainsight_px_browser/ui-config.json (2)
Learnt from: gs-nwolfe
PR: rudderlabs/rudder-integrations-config#1684
File: src/configurations/destinations/gainsight_px_browser/ui-config.json:109-220
Timestamp: 2024-10-08T15:52:59.819Z
Learning: In `ui-config.json` files, using multiple conditions for the same `configKey` in the `preRequisites`, such as for `AMP_enable-gcm`, is acceptable and commonly used across multiple destinations.
Learnt from: gs-nwolfe
PR: rudderlabs/rudder-integrations-config#1684
File: src/configurations/destinations/gainsight_px_browser/ui-config.json:109-220
Timestamp: 2024-10-07T23:42:32.854Z
Learning: In `ui-config.json` files, using multiple conditions for the same `configKey` in the `preRequisites`, such as for `AMP_enable-gcm`, is acceptable and commonly used across multiple destinations.
🔇 Additional comments (7)
src/configurations/destinations/gainsight_px_browser/db-config.json (5)
17-19: LGTM: Source and message type configuration.
The configuration correctly:
- Limits source type to "web"
- Specifies device mode for web connections
- Supports appropriate message types (identify, track, group)
31-32: LGTM: Security configuration.
The productKey is correctly marked as a secret key, ensuring proper handling of sensitive information.
4-6: Verify processor transformation configuration.
The transformAtV1 setting is set to "processor". Please ensure this aligns with the existing Gainsight PX destination's transformation logic to maintain consistency when merging.
#!/bin/bash
# Check transformation configuration in existing Gainsight PX destination
rg -g "db-config.json" '"transformAtV1"' ./configurations/destinations/gainsight_px/20-30: Verify consent management implementation.
The configuration includes consent management features:
oneTrustCookieCategoriesin defaultConfigconsentManagementin web config
Please ensure this aligns with Gainsight PX's consent management requirements and documentation.
7-16: Review the completeness of configuration keys.
The configuration includes essential keys for device mode operation. However, when merging with the existing destination:
- Ensure no cloud mode keys are accidentally removed
- Consider adding validation for required keys
✅ Verification successful
Configuration keys are correctly separated between browser and cloud modes
After comparing both configurations, I can confirm that:
- The browser mode configuration (
gainsight_px_browser) correctly includes device-mode specific keys likeproductKey,dataCenter, and event filtering options. - The cloud mode configuration (
gainsight_px) maintains its separate set of keys includingapiKey,productTagKey, and attribute mappings. - Common keys like
oneTrustCookieCategoriesandconsentManagementare present in both configurations.
No cloud mode keys were accidentally removed, and the configuration separation is properly maintained between the two integration modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Compare with existing Gainsight PX destination keys
echo "Checking existing Gainsight PX configuration keys:"
rg -g "db-config.json" '"includeKeys"' -A 10 ./configurations/destinations/gainsight_px/
Length of output: 283
Script:
#!/bin/bash
# Let's try searching for all db-config.json files in the repository
echo "Finding all db-config.json files:"
fd "db-config.json"
# Search for any Gainsight PX related configuration files
echo -e "\nSearching for Gainsight PX related files:"
fd -t f -i "gainsight.*px"
# Let's also check the content of the browser destination config
echo -e "\nChecking browser destination config content:"
cat src/configurations/destinations/gainsight_px_browser/db-config.json
Length of output: 21049
Script:
#!/bin/bash
# Let's check the content of the regular Gainsight PX destination config
echo "Checking Gainsight PX destination config content:"
cat src/configurations/destinations/gainsight_px/db-config.json
# Also check if there are any cloud mode specific configurations
echo -e "\nSearching for cloud mode configurations in Gainsight PX files:"
rg -i "cloud.*mode" src/configurations/destinations/gainsight_px/
rg -i "cloud.*mode" src/configurations/destinations/gainsight_px_browser/
Length of output: 3313
src/configurations/destinations/gainsight_px_browser/ui-config.json (2)
43-53: LGTM: Native SDK configuration is clear and appropriate.
The default value of true for device-mode event sending aligns well with the integration's purpose.
109-220: LGTM: Comprehensive consent management configuration.
The consent settings section provides:
- Clear guidance about using category IDs over names
- Flexible consent management provider options
- Support for environment variables in category IDs
- Proper feature flag controls
What are the changes introduced in this PR?
New Gainsight PX device mode integration
What is the related Linear task?
n/a
Please explain the objectives of your changes below
The intention is to allow customers to load the Gainsight PX javascript SDK in the browser and send events directly to Gainsight PX from the client side. This new integration will likely be used more than the existing home-grown Gainsight PX integration that uses the REST API to send events from server-to-server.
To avoid conflicts with the existing Gainsight PX integration, I used the name of "GAINSIGHT_PX_BROWSER".
There are corresponding PR's to the sdk-js and docs repos.
Docs PR: rudderlabs/rudderstack-docs#1193
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
[n/a] My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Documentation