-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/nielsen dcr #45
Conversation
🦋 Changeset detectedLatest commit: 44bc3e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
options?: NielsenOptions, | ||
country?: NielsenCountry | ||
) { | ||
if (country == NielsenCountry.CZ) { |
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.
If I understand correctly CZ country should correlate to DCR and otherwise we have DTVR. Can they ever be used at the same time?
Should we prefer the check of DTVR/DCR as this is used in the other parts of the code? Or would we need a check that chosen country and DCR/DTVR is possible?
Not important but the two snippets below seem very similar. Could this be simplified/combined in some way?
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.
For now I think the implementation allows all combinations of possible values for dtvrEnabled
, dcrEnabled
and country
are possible. However, at the moment I think DTVR is US only (see my other reply on your Types.ts comment). To be confirmed.
Indeed the snippets look very similar. The CZ Nielsen rep instructed me to use separate snippets as per the documentation for each country, just in case. I also asked the US based Nielsen rep if there's any chance to use a single snippet.
The only function we use in the integration is ggPM
, which all the snippets I could find in the engineering portal offer and the endpoint to report to is always the same. The only obvious differences I spotted were variable names, surrounding try catch blocks with error logging in the CZ snippet and some snippets that don't have the trackEvent
function. Also, within the same integration guide pages, the snippet is the same for DCR and DTVR (if present).
}; | ||
|
||
/* | ||
* Countries for which (1) Nielsen provides DCR Browser SDKs and (2) the corresponding SDK was tested with this integration. |
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.
Is the country only for DCR? I thought DTVR also has some country specific differences.
It also seems that at some places we associate US with DTVR at the moment.
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.
I made that assumption yes. [Main DCR & DTVR page](The https://engineeringportal.nielsen.com/wiki/DCR_%26_DTVR) mentions US in the title, whereas the International page doesn't mention DTVR. I asked a Nielsen representative to confirm this.
Can you highlight the places where US seems to be associated with DTVR? The only one I can think of is the default values in the configuration for dtvrEnabled
(true
) and country
("US"
) , but this is just to prevent breaking functionality for existing users of this connector.
switch (this.country) { | ||
case NielsenCountry.US: { | ||
const { type, vidtype, assetid, ...updateableParameters } = metadata; | ||
console.log(`[NIELSEN] updateMetadata: ${{ type, vidtype, assetid }} will not be updated`); |
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.
Do we want to keep this log? Or was this for testing?
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.
I thought it made sense to warn developers about the fact that not any passed metadata can be updated mid playback. I have put it behind a check for nol_sdkDebug === "debug"
, so it only shows if logging is enabled for the Nielsen calls also.
Add DCR support for US and International (CZ) SDKs
The connector by default serves as a DTVR integration (US), as it did before this PR. You can now pass a NielsenConfiguration to the constructor of NielsenConnector where you specify which functionality you need. E.g. For the Czech Republic DCR integration, initialize the connector as follows:
The focus was on the Czech Republic DCR implementation for this PR. You can find the documentation here and on the Nielsen engineering portal. However, a Nielsen representative pointed out that the main differences between the DCR implementations are mainly in the metadata requirements, so typings and helper functions to build metadata for US based customers have also been included. Note that also the static queue snippet differs per configured country's documentation. This is also reflected in this integration.
The existing DTVR functionality should have remained intact, but is now behind configuration flags. Note that some small changes have been made that impact DTVR too:
type
,vidtype
, orassetid
parameters , as per the documentation on this eventadbegin
listener, the current ad is retrieved through the event playload instead of the Ads API