Events: Inject tracker if events are enabled #3803

Pubmatic-Dhruv-Sonone · 2024-07-09T16:18:46Z

No description provided.

guscarreon

I like exchange/exchangetest/events-vast-account-off-request-on.json and exchange/exchangetest/events-vast-account-on-request-off.json but, can we add unit tests for modifyBidVAST, convertToVastEvent, and appendURLs in exchange/events_test.go please?

guscarreon · 2024-07-31T15:58:36Z

exchange/events.go

+
+	for _, event := range events.VASTEvents {
+		switch event.CreateElement {
+		// TODO: Prebid currently uses config.ExternalURL for impression tracking, decprecated in favor of config.Event


Nitpick: typo in the word "deprecated"

@guscarreon Add unit tests and fixed typo.

guscarreon

With this PR, can we finally enable? Or should we keep line 62 below?

60 // Events indicates the various types of events to be captured typically for injecting tracker URLs
61 // within the VAST XML
62 // Don't enable this feature. It is still under developmment. Please follow https://github.com/prebid/prebid-server/issues/1725 for more updates
63 type Events struct {
64     Enabled    bool        `mapstructure:"enabled" json:"enabled"`
65     DefaultURL string      `mapstructure:"default_url" json:"default_url"`
66     VASTEvents []VASTEvent `mapstructure:"vast_events" json:"vast_events,omitempty"`
67 }
config/events.go

@bsardo you edited line 64 some time ago, are we ready to enable?

bsardo · 2024-09-16T17:22:09Z

With this PR, can we finally enable? Or should we keep line 62 below?

60 // Events indicates the various types of events to be captured typically for injecting tracker URLs
61 // within the VAST XML
62 // Don't enable this feature. It is still under developmment. Please follow https://github.com/prebid/prebid-server/issues/1725 for more updates
63 type Events struct {
64     Enabled    bool        `mapstructure:"enabled" json:"enabled"`
65     DefaultURL string      `mapstructure:"default_url" json:"default_url"`
66     VASTEvents []VASTEvent `mapstructure:"vast_events" json:"vast_events,omitempty"`
67 }
config/events.go

@bsardo you edited line 64 some time ago, are we ready to enable?

I'm not sure off the top of head. @Pubmatic-Dhruv-Sonone are you expecting to open additional PRs to complete this feature?

Pubmatic-Dhruv-Sonone · 2024-09-17T05:32:57Z

@bsardo I will be creating new PR for Prometheus stats and moving existing impression tracker URL to event framework.

bsardo · 2024-10-18T13:00:21Z

exchange/events.go

+		// TODO: Prebid currently uses config.ExternalURL for impression tracking, deprecated in favor of config.Event
+		// case config.ImpressionVASTElement:
+		// 	ve.Impressions = appendURLs(ve.Impressions, event, events.DefaultURL)


I assume you're going to uncomment this when the feature is enabled, is that right?

Currently cfg.ExternalURL is used for impression tracker And the event injection framework uses account.Event.URLS. Either we will have to deprecated cfg.ExternalURL and have impression as part of account.Event.URLS or set cfg.ExternalURL in account.Event.URLS during processing.

We can't deprecate a config until the next major release. In the config layer, can we have cfg.ExternalURL be the default value?

@SyntaxNode Do you want to add cfg.ExternalURL as default config only for impression tracker or all trackers events?
Currently impression trackers are added in ModifyVastXmlString and events injector code does not add impression tracker currently as that part is commented. I was going to create a separate PR for removing ModifyVastXmlString function and moving impression tracker injection functionality to events injector framework, there I can use cfg.ExternalURL for impression tracker inside convertToVastEvent. If needed I can do that as part of this PR.

@Pubmatic-Dhruv-Sonone if it is easy to do, I would make cfg.ExternalURL the default for all tracker events. I suggest moving forward with your plan of creating a separate PR to remove ModifyVastXmlString with the corresponding functionality moving to the events injector framework and using cfg.ExternalURL as the default.

bsardo · 2024-10-18T13:08:35Z

exchange/events_test.go

+	}
+}
+
+func TestAppendURLs(t *testing.T) {


Can you add a test case where urls is nil and one where event.URLs is nil?

bsardo · 2024-10-18T13:41:46Z

exchange/events_test.go

@@ -192,3 +195,296 @@ func Test_isEventAllowed(t *testing.T) {
 		})
 	}
 }
+
+func TestConvertToVastEvent(t *testing.T) {


Can you add one more test case here with GeneratedBidID set?

Pubmatic-Dhruv-Sonone · 2024-10-21T06:32:22Z

@bsardo Addressed comments. Please check.

bsardo · 2024-11-04T16:15:56Z

Hi @Pubmatic-Dhruv-Sonone, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from v2 to v3.
For example:

import (
    "github.com/prebid/prebid-server/v3/adapters"
)

As a result, please merge with master (no rebase) and then ensure all Prebid Server package import references in the files you’ve changed are v3 so we can resume reviewing. Thanks!

Pubmatic-Dhruv-Sonone · 2024-11-06T16:00:59Z

@bsardo Merged the latest prebid master. Please check.

SyntaxNode · 2024-11-15T21:09:16Z

exchange/events.go

+		// TODO: Prebid currently uses config.ExternalURL for impression tracking, deprecated in favor of config.Event
+		// case config.ImpressionVASTElement:
+		// 	ve.Impressions = appendURLs(ve.Impressions, event, events.DefaultURL)


We can't deprecate a config until the next major release. In the config layer, can we have cfg.ExternalURL be the default value?

SyntaxNode · 2024-11-15T21:12:32Z

exchange/events_test.go

+		expected injector.VASTEvents
+	}{
+		{
+			name: "Error event",


Nitpick: t.Run does not allow test names to have spaces and will auto fill them in. To make it easier to associate a test failure, please do not use spaces in test names. Consider formats like error-event.

SyntaxNode · 2024-11-15T21:20:41Z

exchange/events_test.go

+				Bid: &openrtb2.Bid{
+					ID:   "bid2",
+					NURL: "http://nurl.com",
+					AdM:  "<VAST version=\"4.0\" xmlns=\"http://www.iab.com/VAST\"><Ad id=\"20011\" sequence=\"1\" conditionalAd=\"false\"><Wrapper followAdditionalWrappers=\"0\" allowMultipleAds=\"1\" fallbackOnNoAd=\"0\"><AdSystem version=\"4.0\"><![CDATA[iabtechlab]]></AdSystem><Error><![CDATA[http://example.com/error]]></Error><Impression id=\"Impression-ID\"><![CDATA[http://example.com/track/impression]]></Impression><Impression><![CDATA[http://example.com/event?t=imp&b=generatedBidID&a=account1&bidder=bidder2&f=b&int=integration1&ts=1234567890]]></Impression><Creatives><Creative id=\"5480\" sequence=\"1\" adId=\"2447226\"><CompanionAds><Companion id=\"1232\" width=\"100\" height=\"150\" assetWidth=\"250\" assetHeight=\"200\" expandedWidth=\"350\" expandedHeight=\"250\" apiFramework=\"VPAID\" adSlotID=\"3214\" pxratio=\"1400\"><StaticResource creativeType=\"image/png\"><![CDATA[https://www.iab.com/wp-content/uploads/2014/09/iab-tech-lab-6-644x290.png]]></StaticResource><CompanionClickThrough><![CDATA[https://iabtechlab.com]]></CompanionClickThrough><CompanionClickThrough><![CDATA[http://tracking.url]]></CompanionClickThrough><CompanionClickThrough><![CDATA[http://default.url]]></CompanionClickThrough></Companion></CompanionAds></Creative></Creatives><VASTAdTagURI><![CDATA[https://raw.githubusercontent.com/InteractiveAdvertisingBureau/VAST_Samples/master/VAST%204.0%20Samples/Inline_Companion_Tag-test.xml]]></VASTAdTagURI></Wrapper></Ad></VAST>",


Can the AdM for the test be simplified / slimmed down to just the essentials to make the test case clearer?

Agreed. Can you reduce the noise in all of the AdMs in this test?

@Pubmatic-Dhruv-Sonone I just wanted to check in and see if all or most of the AdM is necessary or if it can be slimmed down.

SyntaxNode · 2024-11-15T21:21:35Z

injector/injector.go

@@ -11,7 +11,7 @@ import (
 )

 const (
-	emptyAdmResponse = `<VAST version="3.0"><Ad><Wrapper><AdSystem>prebid.org wrapper</AdSystem><VASTAdTagURI><![CDATA[%s]]></VASTAdTagURI><Creatives></Creatives></Wrapper></Ad></VAST>`
+	emptyAdmResponse = `<VAST version="3.0"><Ad><Wrapper><AdSystem>prebid.org wrapper</AdSystem><VASTAdTagURI><![CDATA[%s]]></VASTAdTagURI><Impression></Impression><Creatives></Creatives></Wrapper></Ad></VAST>`


Why is this empty tag added? What happens if this is omitted?

This is as per PRD to return empty Vast in case bid.Adm is empty and NURL is present.
Events PRD

I'm trying to figure out how you arrived at this conclusion. In Event PRD 3.3.2. VAST Modification I see the following requirements:

Otherwise if 'adm' is empty and 'nurl' is present, create a VAST wrapper: <VAST version=\"3.0\"><Ad><Wrapper> <AdSystem>prebid.org wrapper</AdSystem> <VASTAdTagURI><![CDATA[" + bidNurl + "]]></VASTAdTagURI> <Creatives></Creatives> </Wrapper></Ad></VAST>

And also:

Once we know what type of VAST (inline or wrapper) is present: ... b. Wrappers iii. If <Impression> is not present, add the <Impression> tag(s) at the end of the Wrapper element.

I'm a bit confused because I don't see this tag in some of the examples in the spec where a nurl is present.

Can you figure out what Java is doing in this scenario?

bsardo · 2025-02-17T03:07:48Z

@Pubmatic-Dhruv-Sonone see my latest comments.

Pubmatic-Dhruv-Sonone added 3 commits July 4, 2024 15:26

Integrate event tracker with auction logic

bfcda5a

Updating UT

7a114da

Skipping tracker events for impression

65c9114

hhhjort requested a review from AlexBVolcy July 10, 2024 17:06

hhhjort assigned AlexBVolcy Jul 10, 2024

bsardo assigned bsardo and guscarreon and unassigned AlexBVolcy Jul 29, 2024

guscarreon reviewed Jul 31, 2024

View reviewed changes

Adding UTs

6f3e323

guscarreon reviewed Sep 10, 2024

View reviewed changes

bsardo reviewed Oct 18, 2024

View reviewed changes

bsardo changed the title ~~Inject event tracker if events are enabled~~ Events: Inject tracker if events are enabled Oct 18, 2024

Adding new UTs

1472283

bsardo previously approved these changes Oct 22, 2024

View reviewed changes

Merging latest prebid master

27379f3

Pubmatic-Dhruv-Sonone dismissed bsardo’s stale review via 27379f3 November 6, 2024 15:59

bsardo previously approved these changes Nov 8, 2024

View reviewed changes

SyntaxNode reviewed Nov 15, 2024

View reviewed changes

Fixing UT

82c20a1

Pubmatic-Dhruv-Sonone dismissed bsardo’s stale review via 82c20a1 November 22, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events: Inject tracker if events are enabled #3803

Events: Inject tracker if events are enabled #3803

Pubmatic-Dhruv-Sonone commented Jul 9, 2024

guscarreon left a comment

guscarreon Jul 31, 2024

Pubmatic-Dhruv-Sonone Aug 30, 2024

guscarreon left a comment

bsardo commented Sep 16, 2024

Pubmatic-Dhruv-Sonone commented Sep 17, 2024

bsardo Oct 18, 2024

Pubmatic-Dhruv-Sonone Oct 21, 2024

SyntaxNode Nov 15, 2024

Pubmatic-Dhruv-Sonone Nov 22, 2024 •

edited

Loading

bsardo Jan 13, 2025

bsardo Oct 18, 2024

Pubmatic-Dhruv-Sonone Oct 21, 2024

bsardo Oct 18, 2024

Pubmatic-Dhruv-Sonone Oct 21, 2024

Pubmatic-Dhruv-Sonone commented Oct 21, 2024

bsardo commented Nov 4, 2024

Pubmatic-Dhruv-Sonone commented Nov 6, 2024

SyntaxNode Nov 15, 2024

SyntaxNode Nov 15, 2024

Pubmatic-Dhruv-Sonone Nov 22, 2024

SyntaxNode Nov 15, 2024

bsardo Dec 2, 2024

bsardo Jan 23, 2025

SyntaxNode Nov 15, 2024

Pubmatic-Dhruv-Sonone Nov 22, 2024

bsardo Jan 23, 2025 •

edited

Loading

bsardo commented Feb 17, 2025

Events: Inject tracker if events are enabled #3803

Are you sure you want to change the base?

Events: Inject tracker if events are enabled #3803

Conversation

Pubmatic-Dhruv-Sonone commented Jul 9, 2024

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

bsardo commented Sep 16, 2024

Pubmatic-Dhruv-Sonone commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pubmatic-Dhruv-Sonone Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pubmatic-Dhruv-Sonone commented Oct 21, 2024

bsardo commented Nov 4, 2024

Pubmatic-Dhruv-Sonone commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

bsardo commented Feb 17, 2025

Pubmatic-Dhruv-Sonone Nov 22, 2024 •

edited

Loading

bsardo Jan 23, 2025 •

edited

Loading