-
Notifications
You must be signed in to change notification settings - Fork 284
[MAIN] [STRATCONN] Added Session Plugin for Amplitude #3056
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3056 +/- ##
==========================================
+ Coverage 78.07% 79.55% +1.47%
==========================================
Files 1064 1125 +61
Lines 19612 20873 +1261
Branches 3767 4023 +256
==========================================
+ Hits 15313 16606 +1293
- Misses 3015 3572 +557
+ Partials 1284 695 -589 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -32,14 +52,53 @@ const action: BrowserActionDefinition<Settings, {}, Payload> = { | |||
title: 'Session Plugin', | |||
description: 'Generates a Session ID and attaches it to every Amplitude browser based event.', | |||
platform: 'web', | |||
hidden: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add any documentation and ask our customers not to disable this?
default: false, | ||
required: false, | ||
description: | ||
'Generate session start and session end events. This is useful for tracking user sessions. NOTE: This will generate a Segment track() event which will also get send to all Destinations connected to the JS Source' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Generate session start and session end events. This is useful for tracking user sessions. NOTE: This will generate a Segment track() event which will also get send to all Destinations connected to the JS Source' | |
'Generate session start and session end track() events. These events will be sent to the Javascript Source and will be forwarded on to any connected Destinations.' |
@@ -102,6 +102,14 @@ const destination: DestinationDefinition<Settings> = { | |||
} | |||
], | |||
default: 'north_america' | |||
}, | |||
trackSession: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this referenced? I don't see it referenced anywhere in this PR.
packages/browser-destinations/destinations/amplitude-plugins/src/sessionId/index.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AnkitSegment this mostly looks good to me.
Please see inline comments.
I didn't understand why a new Setting field was added to the Cloud Mode Destination. Can you explain please?
I've asked some people with AJS knowledge to take a look at this PR. |
A summary of your pull request, including the what change you're making and why.
In Amplitude, I’ve updated the session plugin by setting hidden to false, allowing users to configure the session time and enable or disable allowSessionTracking. With this change, users can now also track session start and session end events.
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.