Vast Tracker Injection Design Review [DRAFT] #3039

Pubmatic-Dhruv-Sonone · 2023-08-21T12:32:54Z

This PR consist of different approaches for vast event tracker injection.

BenchMark:

// BenchmarkXMLParser-12    	        10081	         114797 ns/op        38260 B/op	     333 allocs/op
// BenchmarkXMLEncoding-12              10802	         109733 ns/op        38601 B/op	     286 allocs/op
// BenchmarkStructUnmarshal-12          4402	         239360 ns/op        72024 B/op	     829 allocs/op
// BenchmarkStringParser-12    	        34964	         31871  ns/op        28325 B/op	     30 allocs/op
// BenchmarkETree-12    	        5698	         180206 ns/op        61769 B/op	     792 allocs/op

AlexBVolcy · 2023-08-29T18:44:30Z

exchange/injector/etree/injector.go

+	}
+}
+
+func (builder *TrackerInjector) Build(vastXML string, NURL string) string {


Can repeated strings like "TrackingEvents", "VideoClicks", "Tracking", be extracted to constants?

AlexBVolcy · 2023-08-29T18:46:52Z

exchange/injector/etree/injector.go

+				inline.AddChild(impression)
+			}
+			for _, url := range builder.events.Errors {
+				url, _ := builder.replacer.Replace(url, builder.provider)


I think you shouldn't ignore the error being returned here

AlexBVolcy · 2023-08-29T18:47:07Z

exchange/injector/etree/injector.go

+					builder.provider.PopulateEventMacros("creativeId", "", "")
+
+					for _, url := range builder.events.VideoClicks {
+						url, _ := builder.replacer.Replace(url, builder.provider)


I think you shouldn't ignore the error being returned here

AlexBVolcy · 2023-08-29T18:47:28Z

exchange/injector/etree/injector.go

+					builder.provider.PopulateEventMacros("creativeID", "", "")
+
+					for _, url := range builder.events.NonLinearClickTracking {
+						url, _ := builder.replacer.Replace(url, builder.provider)


Same error related comment

AlexBVolcy · 2023-08-29T18:48:09Z

exchange/injector/etree/injector_test.go

+// goos: darwin
+// goarch: amd64
+// pkg: github.com/prebid/prebid-server/exchange/injector/etree
+// cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
+// BenchmarkETree-12    	    5698	    180206 ns/op	   61769 B/op	     792 allocs/op
+// PASS
+// ok  	github.com/prebid/prebid-server/exchange/injector/etree	1.497s


Will these comments be removed when this ready to be taken off as draft?

AlexBVolcy · 2023-08-29T18:58:21Z

exchange/injector/xml_parser/injector.go

+}
+
+func parseVastXML(vastXML []byte) []Ad {
+


Nitpick: Can you remove blank line here

AlexBVolcy · 2023-08-29T18:58:24Z

exchange/injector/xml_parser/injector.go

+}
+
+func (builder *TrackerInjector) Build(vastXML string, NURL string) string {
+


Nitpick: Can you remove blank line here

AlexBVolcy · 2023-08-29T18:58:32Z

exchange/injector/xml_parser/injector.go

+}
+
+func (builder *TrackerInjector) addEvent(buf *strings.Builder, pair pair) {
+


Nitpick: Can you remove blank line here

AlexBVolcy · 2023-08-29T19:00:51Z

exchange/injector/vaststruct/injector.go

+	// Nodes   []Node     `xml:",any"`
+}
+
+func main() {


This main function needs code coverage

AlexBVolcy · 2023-08-29T19:01:26Z

exchange/injector/xml_encoder/injector.go

+					outputXML.WriteString("</TrackingEvents>")
+					encoder.EncodeToken(tt)
+				}
+			case "Linear":


This linear case needs code coverage

SyntaxNode · 2023-08-31T04:14:37Z

@AlexBVolcy I think you may have misunderstood the intent of this PR. It proposes several algorithms for implementing XML injector tracking. @Pubmatic-Dhruv-Sonone is seeking our input on which approach is preferred.

SyntaxNode · 2023-08-31T04:28:18Z

@Pubmatic-Dhruv-Sonone Thank you for implementing on all of the options and including detailed benchmark results.

While StringParser is the clear winner in terms of speed and allocations, I'm worried how it might behave in the case of invalid xml, which may be intentional as an attack on the system. We've had issues a long time ago with string shortcuts for json generation. What do others think about this?

XMLParser and XMLEncoder are essentially tied for first among the options which validate the xml structure. The Encoder has a slight memory advantage with cleaner code IMHO, making it a winner in my view. Rather than constantly flushing the encoder and outputting xml directly to the output buffer, could use the encoder to construct those tags?

Pubmatic-Dhruv-Sonone · 2023-08-31T13:57:59Z

@Pubmatic-Dhruv-Sonone Thank you for implementing on all of the options and including detailed benchmark results.

While StringParser is the clear winner in terms of speed and allocations, I'm worried how it might behave in the case of invalid xml, which may be intentional as an attack on the system. We've had issues a long time ago with string shortcuts for json generation. What do others think about this?

XMLParser and XMLEncoder are essentially tied for first among the options which validate the xml structure. The Encoder has a slight memory advantage with cleaner code IMHO, making it a winner in my view. Rather than constantly flushing the encoder and outputting xml directly to the output buffer, could use the encoder to construct those tags?

@SyntaxNode Updated the XMLEncoder approach to encode tags instead of flushing to output buffer.
Below is the benchmark for the updated approach. The performance has degraded after the changes.

// goos: darwin
// goarch: amd64
// pkg: github.com/prebid/prebid-server/exchange/injector/xml_encoder
// cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
// BenchmarkXMLEncoding-12    	    6666	    158149 ns/op	   53132 B/op	     398 allocs/op
// PASS
// ok  	github.com/prebid/prebid-server/exchange/injector/xml_encoder	1.395s

Also in XMLEncoder approach, sequence of the tags is not maintained as we are injecting child tag just before the end of parent tags.

Ex If vast xml already has a Impression tag then this approach injects new Impression tag before the ending Inline or Wrapper tag. This might break the vast xsd validation as it expected similar tags to be grouped together.

The StringParser approach cannot validate vast xml. Also if some attributes are to be extracted from the tags for tracker macro replacement , we will have to add separate handling for each attribute.

Pubmatic-Dhruv-Sonone · 2023-09-20T19:38:55Z

@SyntaxNode I changed XMLEncoder approach as suggested by you, Do you have anymore thoughts regarding the approaches?

SyntaxNode · 2023-10-23T01:33:25Z

@Pubmatic-Dhruv-Sonone Since I think we've selected a design, can we close this PR? You can open a new PR with the final code when you're ready.

Tracker Injection Desing

ced1e64

Pubmatic-Dhruv-Sonone changed the title ~~Vast Tracker Injection Design Review~~ Vast Tracker Injection Design Review [DRAFT] Aug 21, 2023

hhhjort requested review from SyntaxNode and AlexBVolcy August 21, 2023 17:52

hhhjort assigned SyntaxNode and AlexBVolcy Aug 21, 2023

AlexBVolcy reviewed Aug 29, 2023

View reviewed changes

Updating xml_encoder method

c07b8fb

Pubmatic-Dhruv-Sonone closed this Nov 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vast Tracker Injection Design Review [DRAFT] #3039

Vast Tracker Injection Design Review [DRAFT] #3039

Pubmatic-Dhruv-Sonone commented Aug 21, 2023 •

edited by SyntaxNode

Loading

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

AlexBVolcy Aug 29, 2023

SyntaxNode commented Aug 31, 2023

SyntaxNode commented Aug 31, 2023

Pubmatic-Dhruv-Sonone commented Aug 31, 2023 •

edited

Loading

Pubmatic-Dhruv-Sonone commented Sep 20, 2023

SyntaxNode commented Oct 23, 2023

		}

		func (builder *TrackerInjector) Build(vastXML string, NURL string) string {

		}

		func (builder TrackerInjector) addEvent(buf strings.Builder, pair pair) {

Vast Tracker Injection Design Review [DRAFT] #3039

Vast Tracker Injection Design Review [DRAFT] #3039

Conversation

Pubmatic-Dhruv-Sonone commented Aug 21, 2023 • edited by SyntaxNode 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

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

SyntaxNode commented Aug 31, 2023

SyntaxNode commented Aug 31, 2023

Pubmatic-Dhruv-Sonone commented Aug 31, 2023 • edited Loading

Pubmatic-Dhruv-Sonone commented Sep 20, 2023

SyntaxNode commented Oct 23, 2023

Pubmatic-Dhruv-Sonone commented Aug 21, 2023 •

edited by SyntaxNode

Loading

Pubmatic-Dhruv-Sonone commented Aug 31, 2023 •

edited

Loading