refactor: deprecate mixpanel /track endpoint #2833
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2833 +/- ##
===========================================
+ Coverage 87.71% 87.82% +0.11%
===========================================
Files 550 556 +6
Lines 29616 29926 +310
Branches 7060 7146 +86
===========================================
+ Hits 25977 26283 +306
- Misses 3303 3338 +35
+ Partials 336 305 -31 ☔ View full report in Codecov by Sentry. |
|
Kudos, SonarCloud Quality Gate passed! |
Why you have changed to project token? For /import endpoint we need project secret. |
Now /import supports project token, Mixpanel doc is not updated yet but i confirmed with mixpanel team and tested through postman also. |
|
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. |
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes involve significant adjustments to the Mixpanel integration within a codebase, specifically focusing on event tracking and batching logic. The removal of the Changes
Assessment against linked issues
The primary objective from the linked issue is the integration of the latest GATransform artifacts into the master branch of the rudder-transformer repository. While the summary of changes does not explicitly mention "GATransform" artifacts, the detailed adjustments to event tracking and batching, as well as the shift in handling credentials and endpoints, suggest a significant update that could be part of integrating such artifacts. The changes align with an effort to enhance or update the data processing capabilities, which would be in line with the objective of incorporating new or updated artifacts into the repository. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/2833/test-report.html |
1 similar comment
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/2833/test-report.html |
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/v0/destinations/mp/config.js (2 hunks)
- src/v0/destinations/mp/transform.js (4 hunks)
- src/v0/destinations/mp/util.js (2 hunks)
- src/v0/destinations/mp/util.test.js (10 hunks)
- test/integrations/destinations/mp/common.ts (1 hunks)
- test/integrations/destinations/mp/processor/data.ts (78 hunks)
- test/integrations/destinations/mp/router/data.ts (6 hunks)
Files not summarized due to errors (1)
- test/integrations/destinations/mp/processor/data.ts: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- src/v0/destinations/mp/util.js
Additional comments: 10
test/integrations/destinations/mp/common.ts (1)
- 8-8: The
tokenvalue in thesampleDestinationobject has been updated to 'test_api_token'. Ensure this new token value aligns with the expected test configurations and authentication requirements for the/importendpoint.src/v0/destinations/mp/transform.js (3)
- 49-60: The
setImportCredentialsfunction has been updated to support the new authentication method using a project token. Ensure that the credentials are correctly encoded in Base64 and that theAuthorizationheader is properly formatted.- 69-87: The
responseBuilderSimplefunction has been modified to support the new/importendpoint. Verify that the event timestamp validation (within the last 5 years) and the setting of request parameters are correctly implemented for all event types.- 482-488: The batching logic in
processRouterDesthas been updated to accommodate the new/importendpoint. Ensure that the batching of events is correctly handled forengageEvents,groupsEvents, andimportEvents, and that the batch size limits are respected.src/v0/destinations/mp/util.test.js (2)
- 139-148: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [81-145]
The occurrences of
trackEventshave been replaced withimportEvents, and the endpoint URLs have been updated from/trackto/import. Confirm that these changes accurately reflect the new event tracking methodology and that the tests are correctly updated to target the/importendpoint.
- 292-297: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [268-330]
The
combineBatchRequestsWithSameJobIdsfunction test cases have been updated to use the/importendpoint. Ensure that the logic for combining batch requests with the same job IDs is correctly implemented and that the tests accurately reflect the expected behavior.test/integrations/destinations/mp/router/data.ts (4)
- 442-448: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The import statement and the usage of
overrideDestinationandsampleDestinationremain unchanged, ensuring that the test setup utilizes consistent destination configuration across tests.
- 445-445: The
Authorizationheader in the expected output for Mixpanel API calls correctly uses a base64-encodedtest_api_token, aligning with the PR's objective to shift from using a project secret to a project token for authentication. This change is consistent across all test cases, ensuring that the new authentication method is thoroughly tested.Also applies to: 511-511, 578-578, 1164-1164, 1229-1229, 1295-1295
- 442-448: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2-1369]
The test cases provided in the file cover a comprehensive range of scenarios, including different types of calls and settings, ensuring thorough testing of the integration with the Mixpanel destination. The detailed configurations, including the shift to the
/importendpoint and the use of project tokens for authentication, align with the PR's objectives and demonstrate a thoughtful approach to testing the changes.
- 442-448: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1360-1369]
The test case validating the error handling for missing group keys, resulting in a
400status code, effectively ensures that the integration correctly identifies and reports configuration errors. This test case is crucial for verifying the robustness of the error handling mechanisms in the integration.
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/2833/test-report.html |
|
|
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. |
|
This reverts commit 1bfcd27.
* refactor: deprecate mixpanel /track endpoint * fix: used token in place of apiSecret for merge call * fix: no-case-declarations
Description of the change
/trackendpoint and implemented track event calls through the existing/importendpoint. This change offers a higher batch limit, supports gzip compression, enables the ingestion of historical data, and provides several other enhancements. For more details, refer to mixpanel /track vs /import comparison doc./trackendpoint, all calls will be sent via the/importendpoint. As a result, in the identify call, we used to perform the merging of known (userId) and anonymous (anonymousId) users through an additional call using the /import endpoint. The earlier decision to use the /track versus /import endpoint was based on configuration. However, now that all calls will be sent using the /import endpoint, Customers should expect additional merge calls in their profile feed after this change.project secretfor /import API as it is in the deprecating stage at Mixpanel, Instead we can useproject token(which is a required field in webapp)Type of change
Related issues
Checklists
Development
Code review
Summary by CodeRabbit