Event Tracking: Macro Replacement Framework. #2629

Pubmatic-Dhruv-Sonone · 2023-03-08T08:21:34Z

BenchMark Results:

// 1) ^BenchmarkStringIndexCachedBasedReplacer$ github.com/prebid/prebid-server/macros
// goos: darwin
// goarch: amd64
// pkg: github.com/prebid/prebid-server/macros
// cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
// BenchmarkStringIndexCachedBasedReplacer-12    	   65348	     17148 ns/op	   12617 B/op	      56 allocs/op
// PASS
// ok  	github.com/prebid/prebid-server/macros	2.446s

// 2) ^BenchmarkGolangReplacer$ github.com/prebid/prebid-server/macros
// goos: darwin
// goarch: amd64
// pkg: github.com/prebid/prebid-server/macros
// cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
// BenchmarkGolangReplacer-12    	   10926	     99165 ns/op	   81887 B/op	     603 allocs/op
// PASS
// ok  	github.com/prebid/prebid-server/macros	2.144s

…trackers

* OTT-421: Unit tests and basic framework for events consumption and validation * OTT-421: Changed eventvalidation interface according to configuration.validate() requirements * OTT-421: Made validate method from eventvalidation.go private Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

* OTT-421: Added note to not enabled this feature. Because, it is still in progress Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

…trackers

…entscoments (#255) * OTT-421: Addressed code review comments Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

…res) (#256) * OTT-421: Fixed test failures Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

* OTT-421: Added warning when events.enabled is true Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

…ents - 2) (#258) * OTT-421: Unit tests and basic framework for events consumption and validation * OTT-421: Reverted unwanted change. * OTT-421: Changed eventvalidation interface according to configuration.validate() requirements * OTT-421: Reverted unwanted change * OTT-421: Made validate method from eventvalidation.go private * OTT-421: Added note to not enabled this feature. Because, it is still in progress * OTT-421: Temporary print statement * OTT-421: Addressed code review comments * OTT-421: Fixed test failures * OTT-421: Added warning when events.enabled is true * OTT-421: Addressed code review comments * OTT-421: Disabled event validations * OTT-421: Reverted stored_requests/data/by_id/accounts/test.json * OTT-421: Removed unwanted comment Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

…trackers

* OTT-421: Disabled event validations Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

…trackers

…ic-OpenWrap/prebid-server into I1725_vast_event_trackers

* OTT-422: Adding support for eventType=vast in analytic endpoint * Adding test cases

…trackers

…_event_trackers

* OTT-485: Macro Replacement framework (#432)

* OTT-485: Removing Template based approach and config

* Removing macro processor config

* OTT-485: Changing macro prefix from PBS_ to PBS-

Pubmatic-Dhruv-Sonone · 2023-03-16T13:20:48Z

@bsardo Can you please review the PR?

Pubmatic-Dhruv-Sonone · 2023-05-05T12:36:10Z

@Pubmatic-Dhruv-Sonone discussed possible changes in string-based.go. We decided to leave everything as is there. Despite our current PBS is configured to load host and account data at the server start up, we need to support flexibility and give other hosts an ability to "lazy-load" account data. Regarding string replacement mechanism we agreed to move with more productive approach.

May you please add comments to describe the replacement algorithm, so it will be easier to understand in future reviewing and maintenance?

Thank you for your explanations and efforts!

Added comments.

VeronikaSolovei9

Thank you for all changes. Would you consider to modify comments a little? Please see suggestions below.

VeronikaSolovei9 · 2023-05-09T00:32:00Z

macros/string_index_based_replacer.go

+	}
+}
+
+func constructTemplate(url string) urlMetaTemplate {


How about this comment:

// constructTemplate func finds index bounds of all macros in an input string where macro format is ##data##. // constructTemplate func returns two arrays with start indexes and end indexes for all macros found in the input string. // Start index of the macro points to the index of the delimiter(##) start. // End index of the macro points to the end index of the delimiter. // For the valid input string number of start and end indexes should be equal, and they should not intersect. // This approach shows better performance results compare to standard GoLang string replacer.

VeronikaSolovei9 · 2023-05-09T00:48:33Z

macros/string_index_based_replacer.go

+// Replace will replace the macros in the given  event url.
+// Check if urlMetaTemplate exist in cache for given url if not construct the template (start and end index info of each macro) and store in cache.
+// Iterate over the startingIndices in the template and for each starting index and corresponding ending index, extract macro from the url.
+// Get the value of the macro from the macroProvider.
+// Prepend the url string present before the macro to the macro value and append the remaining url.


Would you consider to modify it to:

// Replace function replaces macros in a given string with the data from macroProvider and returns modified input string. // If a given string was previously processed this function fetches its metadata from the cache. // If input string is not found in cache then template metadata will be created. // Iterates over start and end indexes of the template arrays and extracts macro name from the input string. // Gets the value of the extracted macro from the macroProvider. Replaces macro with corresponding value.

VeronikaSolovei9

Thank you for all changes. Would you consider to modify comments a little? Please see suggestions above.

mansinahar

@Pubmatic-Dhruv-Sonone Looking great other than very minor comments

mansinahar · 2023-05-09T02:17:19Z

macros/string_index_based_replacer.go

+		if endingIndex == -1 {
+			break
+		}
+		endingIndex = endingIndex + startIndex // offset adjustment (Delimiter inclusive)


Shouldn't this be

endingIndex = startIndex + endingIndex - 1

the endingIndex returned by the strings.Index function on L48 would be the index of first ending delimiter char not the ending index of the macro key.

mansinahar · 2023-05-09T02:22:37Z

macros/string_index_based_replacer.go

+		if value != "" {
+			result.WriteString(value)
+		}
+		currentIndex = index + len(macro) + delimLen


Wouldn't it be easier to just do

currentIndex = endingIndex + delimLen

currentIndex = endingIndex + delimLen won't work after making changes as per above comment.

currentIndex = endingIndex + 1 + delimLen will work., but the current implement looks more easier to understand to me.

Should I keep it as it is or change to currentIndex = endingIndex + 1 + delimLen?

mansinahar · 2023-05-09T18:19:38Z

macros/provider_test.go

+
+func TestGetMacro(t *testing.T) {
+	type args struct {
+		key            string


@Pubmatic-Dhruv-Sonone As mentioned in my previous comment,

In this case we just wanna test the logic in GetMacro function and that just retrieves and returns the macro in the map.

So you don't need to add a test case for each macro key in here. 1-2 keys are enough. The logic we need to check is that if macro is present in provider, it will return it. If not, it won't. GetMacro knows nothing about the specific keys so test cases for each macro key don't add much value here.

mansinahar · 2023-05-09T18:21:06Z

router/router.go

@@ -214,7 +215,8 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R
 	}

 	planBuilder := hooks.NewExecutionPlanBuilder(cfg.Hooks, repo)
-	theExchange := exchange.NewExchange(adapters, cacheClient, cfg, syncersByBidder, r.MetricsEngine, cfg.BidderInfos, gdprPermsBuilder, tcf2CfgBuilder, rateConvertor, categoriesFetcher, adsCertSigner)
+	theExchange := exchange.NewExchange(adapters, cacheClient, cfg, syncersByBidder, r.MetricsEngine, cfg.BidderInfos, gdprPermsBuilder, tcf2CfgBuilder, rateConvertor, categoriesFetcher, adsCertSigner, macros.NewStringIndexBasedReplacer())


I recommend instantiating macroReplacer before this line and then use that variable as parameter to NewExchange so that it is explicit and clear to the reader what macroReplcer is being used

mansinahar

@Pubmatic-Dhruv-Sonone Looking great other than very minor comments

hhhjort · 2023-05-18T17:08:08Z

Looks like there are.a few merge conflicts to resolve.

mansinahar

@Pubmatic-Dhruv-Sonone LGTM other than the minor comment about parameter ordering in one of the assert.Equal call

mansinahar · 2023-05-24T19:29:23Z

macros/provider_test.go

+			for key := range tt.want {
+				output[key] = b.GetMacro(key)
+			}
+			assert.Equal(t, output, tt.want, tt.name)


The order here for assert.Equal arguments needs to be expected then actual. So, can you please change it to:

assert.Equal(t, tt.want, output, tt.name)

Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

pm-shriprasad-marathe and others added 20 commits January 31, 2022 11:44

Merge remote-tracking branch 'upstream/master' into I1725_vast_event_…

882febf

…trackers

OTT-421 : Added under development feature note (#244)

c4d8cf9

* OTT-421: Added note to not enabled this feature. Because, it is still in progress Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

Merge remote-tracking branch 'upstream/master' into I1725_vast_event_…

c4949cb

…trackers

OTT-421: Host level configurations for video events (Code Review comm…

0510354

…entscoments (#255) * OTT-421: Addressed code review comments Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

OTT-421: Host level configurations for video events (Fixed Test failu…

2ba4def

…res) (#256) * OTT-421: Fixed test failures Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

OTT-421: Added warning when events.enabled=true (#257)

7766601

* OTT-421: Added warning when events.enabled is true Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

Merge remote-tracking branch 'upstream/master' into I1725_vast_event_…

58003bd

…trackers

OTT-421: Host Level Configuration (Code Review addressed) (#261)

3042288

* OTT-421: Disabled event validations Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

Merge 'prebid/master' into I1725_vast_event_trackers

51111e1

Merge remote-tracking branch 'upstream/master' into I1725_vast_event_…

f49f08c

…trackers

Merge branch 'I1725_vast_event_trackers' of https://github.com/PubMat…

a487988

…ic-OpenWrap/prebid-server into I1725_vast_event_trackers

Adding support for EventType = Vast in Analytics Endpoint (#262)

5db8af4

* OTT-422: Adding support for eventType=vast in analytic endpoint * Adding test cases

Improving unit test coverage for event_test.go

308312a

Fixing unit test in event_test.go

c748787

Merge remote-tracking branch 'upstream/master' into I1725_vast_event_…

93f5292

…trackers

Merge branch 'prebid/master' into I1725_vast_event_trackers

a5d479c

Merge remote-tracking branch 'prebid-upstream/master' into I1725_vast…

03b0ff7

…_event_trackers

OTT-485: Macro Replacement Framework (#445)

04cac98

* OTT-485: Macro Replacement framework (#432)

Pubmatic-Dhruv-Sonone mentioned this pull request Mar 8, 2023

Event Tracking - Macro Replacement Framework #2623

Closed

SyntaxNode changed the title ~~I1725: Macro Replacement Framework.~~ Event Tracking: Macro Replacement Framework. Mar 8, 2023

bsardo self-assigned this Mar 8, 2023

Pubmatic-Dhruv-Sonone added 3 commits March 14, 2023 22:03

Removing Template Based Approach and Config (#450)

d433459

* OTT-485: Removing Template based approach and config

OTT-485: Removing macro processor config

d3b7c98

* Removing macro processor config

Changing macro prefix from PBS_ to PBS- (#456)

43b4b72

* OTT-485: Changing macro prefix from PBS_ to PBS-

bsardo assigned SyntaxNode and mansinahar and unassigned bsardo Apr 3, 2023

Pubmatic-Dhruv-Sonone added 13 commits May 5, 2023 14:26

Macro Replacement: Refactoring macro replace

711bce5

Macro Replacement: Adding new test cases for Truncate function

2ab89c7

Macro Replacement: Adding new unit tests

362f6bb

Macro Replacement: Adding new unit test

b37a08b

Macro Replacement: Refactoring unit test

d32d6d0

Macro Replacement: Addressing review comments

7fbb3db

Macro Replacement: Fix the algo

8ab2a7c

Macro Replacement: Fixing algo

6dbb428

Macro Replacement: Moving benchmark to diff file

09dda7f

Merging latest prebid master

30b33e9

Macro Replacement: Adding comments

df57fc5

Running gofmt

c47237b

Running gofmt

832a7d6

VeronikaSolovei9 reviewed May 9, 2023

View reviewed changes

Macro Replacement: Fixing comments

b2fe55e

mansinahar reviewed May 9, 2023

View reviewed changes

Pubmatic-Dhruv-Sonone added 3 commits May 19, 2023 13:49

Macro Replacement: Refactoring algo

a77737d

Macro Replacement: Refactoring algo

12d49f3

Merging latest prebid-master

960837f

mansinahar reviewed May 24, 2023

View reviewed changes

Fixing test cases

8670aad

mansinahar approved these changes May 25, 2023

View reviewed changes

VeronikaSolovei9 approved these changes May 30, 2023

View reviewed changes

bsardo merged commit 9b0fb7f into prebid:master May 30, 2023
3 checks passed

cross-repo-issue-creation bot mentioned this pull request May 30, 2023

Port PR from PBS-Go: Event Tracking: Macro Replacement Framework. prebid/prebid-server-java#2397

Closed

blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023

Event Tracking: Macro Replacement Framework. (prebid#2629)

3c9631b

Co-authored-by: Shriprasad <shriprasad.marathe@pubmatic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event Tracking: Macro Replacement Framework. #2629

Event Tracking: Macro Replacement Framework. #2629

Pubmatic-Dhruv-Sonone commented Mar 8, 2023 •

edited

Pubmatic-Dhruv-Sonone commented Mar 16, 2023

Pubmatic-Dhruv-Sonone commented May 5, 2023

VeronikaSolovei9 left a comment

VeronikaSolovei9 May 9, 2023

VeronikaSolovei9 May 9, 2023

Pubmatic-Dhruv-Sonone May 9, 2023

VeronikaSolovei9 left a comment •

edited

mansinahar left a comment

mansinahar May 9, 2023

Pubmatic-Dhruv-Sonone May 19, 2023

mansinahar May 9, 2023

Pubmatic-Dhruv-Sonone May 19, 2023 •

edited

mansinahar May 9, 2023

mansinahar May 9, 2023

Pubmatic-Dhruv-Sonone May 19, 2023

mansinahar left a comment

hhhjort commented May 18, 2023

mansinahar left a comment •

edited

mansinahar May 24, 2023

Pubmatic-Dhruv-Sonone May 25, 2023

Event Tracking: Macro Replacement Framework. #2629

Event Tracking: Macro Replacement Framework. #2629

Conversation

Pubmatic-Dhruv-Sonone commented Mar 8, 2023 • edited

Pubmatic-Dhruv-Sonone commented Mar 16, 2023

Pubmatic-Dhruv-Sonone commented May 5, 2023

VeronikaSolovei9 left a 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

VeronikaSolovei9 left a comment • edited

Choose a reason for hiding this comment

mansinahar left a 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 May 19, 2023 • edited

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

mansinahar left a comment

Choose a reason for hiding this comment

hhhjort commented May 18, 2023

mansinahar left a comment • edited

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 Mar 8, 2023 •

edited

VeronikaSolovei9 left a comment •

edited

Pubmatic-Dhruv-Sonone May 19, 2023 •

edited

mansinahar left a comment •

edited