Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: SSAI tracking #112

Merged
merged 20 commits into from
Jul 8, 2024
Merged

feat: SSAI tracking #112

merged 20 commits into from
Jul 8, 2024

Conversation

karpov-kir
Copy link
Contributor

@karpov-kir karpov-kir commented Jul 3, 2024 •
edited
Loading

Related to: https://bitmovin.atlassian.net/browse/TA-2617.

  • Extract tracking logic from ConvivaAnalytics into ConvivaAnalyticsTracker
  • Wire ConvivaAnalyticsTracker to player events in ConvivaAnalytics
  • Introduce ConvivaAnalyticsSsai and use ConvivaAnalyticsTracker inside to report server-side ad
  • Add simulation of server-side ad to the example
  • Add tests
Examples

Server-side ad

image

Client-side ad

image

Normal tracking

image

@karpov-kir karpov-kir requested a review from dweinber July 4, 2024 08:58
strangesource
strangesource reviewed Jul 4, 2024
View reviewed changes
Copy link

@strangesource strangesource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look without deeper knowledge in the codebase, looks good to me. 👍
Just some minor remarks.

src/ts/helper/AdHelper.ts Outdated Show resolved Hide resolved
src/ts/helper/AdHelper.ts Show resolved Hide resolved
dweinber
dweinber requested changes Jul 5, 2024
View reviewed changes
Copy link
Member

@dweinber dweinber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for breaking up the large ConvivaAnalytics file! Made reviewing of the feature quite a bit harder, should have probably been a separate PR 😅

There are a few remarks, please address/clarify, thanks.

src/ts/helper/AdHelper.ts Outdated Show resolved Hide resolved
src/ts/helper/AdHelper.ts Show resolved Hide resolved
src/ts/ConvivaAnalytics.ts Show resolved Hide resolved
src/ts/ConvivaAnalyticsTracker.ts Show resolved Hide resolved
src/ts/ConvivaAnalyticsTracker.ts Outdated Show resolved Hide resolved
spec/tests/PlayerEvents.spec.ts Show resolved Hide resolved
@karpov-kir
Copy link
Contributor Author

@dweinber,

Thanks for breaking up the large ConvivaAnalytics file! Made reviewing of the feature quite a bit harder, should have probably been a separate PR 😅

Yeah, I thought about it, but the whole feature is almost in this one file src/ts/ConvivaAnalyticsSsai.ts. 🙂

dweinber
dweinber approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@dweinber dweinber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@karpov-kir karpov-kir merged commit 202fdfd into develop Jul 8, 2024
3 checks passed
@karpov-kir karpov-kir deleted the feature/TA-2617-ssai branch July 8, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strangesource strangesource strangesource approved these changes

@dweinber dweinber dweinber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karpov-kir @dweinber @strangesource