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.

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

Ad lifecycle reporting #68

Merged
merged 18 commits into from
Jun 6, 2024
Merged

Ad lifecycle reporting #68

merged 18 commits into from
Jun 6, 2024

Conversation

matamegger
Copy link
Contributor

@matamegger matamegger commented Jun 4, 2024

Problem

Ad tracking is done using custom events, instead of the conviva provided ad analytics.

Solution

  • Migrate the ad tracking to the ad analytics. This PR provides a basic integration of the ad lifecycle.
  • Adjust existing tests
  • Add new tests for cleanup and the new ad tracking trigger

Notes

Finer grained tracking is implemented in follow up PRs. (To keep PR sizes smaller, or at least the production code changes)

Checklist

  • I added an update to CHANGELOG.md file
  • I ran all the tests in the project and they succeed

@matamegger matamegger self-assigned this Jun 5, 2024
)
}

func onAdFinished() {
videoAnalytics.reportAdBreakEnded()
adAnalytics.reportAdEnded()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is a behaviour change of the integration!!! (and of course the related changes i.e. moving the orignal call into the ad break finished event, and the starting calls in the ad started and ad break started)

@@ -383,6 +387,26 @@ public final class ConvivaAnalytics: NSObject {
}
}

private extension Ad {
var adInfo: [String: Any] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be extended in a follow up PR.

@matamegger matamegger marked this pull request as ready for review June 5, 2024 17:51
@matamegger matamegger requested a review from stonko1994 June 5, 2024 18:30
Copy link
Contributor

@stonko1994 stonko1994 left a comment

Choose a reason for hiding this comment

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

Can you merge develop into this branch. I tried something on the CI to stabilize the unit test execution (not the flaky tests)

}

func onAdBreakFinished(_ event: AdBreakFinishedEvent) {
customEvent(event: event)
videoAnalytics.reportAdBreakEnded()
}

func onDestroy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't comment on internalEndSession():

Should we also end the adAnalytics in there? See this change on the web integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. I couldn't find anything in that regard in the documentation.
I also looked at another integration, and this is not done there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline: Reporting an ended or skipped doesn't really represent the reality. Further, the video analytics and the ad analytics are coupled either way. So reporting a session ended on the video analytics implies that everything else is ended as well.

We will not add preemptive "cleanup".

Sidenote: We tested the reporting anyways, but it does not show up in the dashboard anyway.

@matamegger matamegger requested a review from stonko1994 June 6, 2024 13:37
@matamegger matamegger merged commit c964304 into develop Jun 6, 2024
2 of 3 checks passed
@matamegger matamegger deleted the ad_lifecycle_reporting branch June 6, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants