Update datablocks adapter #2838

jmayor · 2023-06-12T16:58:05Z

Remove dynamic domain with static domain

onkarvhanumante · 2023-06-13T05:53:39Z

@jmayor requesting to also update server urls mentioned in json tests and test files

func TestJsonSamples(t *testing.T) {
	bidder, buildErr := Builder(openrtb_ext.BidderDatablocks, config.Adapter{
		- Endpoint: "http://{{.Host}}/openrtb2?sid={{.SourceId}}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
		+ Endpoint: "http://pbserver.dblks.net/openrtb2?sid={{.SourceId}}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})

	if buildErr != nil {
		t.Fatalf("Builder returned unexpected error %v", buildErr)
	}

	adapterstest.RunJSONBidderTest(t, "datablockstest", bidder)
}

onkarvhanumante · 2023-06-13T05:55:41Z

static/bidder-info/datablocks.yaml

@@ -1,4 +1,4 @@
-endpoint: "http://{{.Host}}/openrtb2?sid={{.SourceId}}"
+endpoint: "http://pbserver.dblks.net/openrtb2?sid={{.SourceId}}"


verified that url is reachable

curl -i http://pbserver.dblks.net/openrtb2 HTTP/1.1 404 Wrong request type: need POST Server: nginx/1.14.1 Date: Tue, 13 Jun 2023 05:54:31 GMT Content-Type: text/plain; charset=UTF-8 Content-Length: 35 Connection: keep-alive X-Fritter: 5.1-5.1-20230411.225543 hashx=8.0 X-Request: F357728-0/S0-0 x-first: good Wrong request type. Must be POST.

Sonali-More-Xandr · 2023-06-21T09:54:25Z

@jmayor Please take a look at the comment by @onkarvhanumante

onkarvhanumante · 2023-06-30T07:30:20Z

@jmayor requesting to also update server urls mentioned in json tests and test files

func TestJsonSamples(t *testing.T) {
	bidder, buildErr := Builder(openrtb_ext.BidderDatablocks, config.Adapter{
		- Endpoint: "http://{{.Host}}/openrtb2?sid={{.SourceId}}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
		+ Endpoint: "http://pbserver.dblks.net/openrtb2?sid={{.SourceId}}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})

	if buildErr != nil {
		t.Fatalf("Builder returned unexpected error %v", buildErr)
	}

	adapterstest.RunJSONBidderTest(t, "datablockstest", bidder)
}

@jmayor could you address above comment

onkarvhanumante · 2023-07-14T06:30:51Z

@jmayor reminder to help with addressing comment

Update test url

jmayor · 2023-07-14T20:42:47Z

Sorry guys - I was travelling and away from internet access. I just committed a change to address it

Sonali-More-Xandr · 2023-07-17T06:26:07Z

@jmayor Please update the server url used in the JSON tests to fix the failing tests

onkarvhanumante · 2023-07-18T05:48:34Z

@jmayor tests are failing with following error

gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
openrtb_ext/imp_datablocks.go

onkarvhanumante · 2023-07-21T06:35:31Z

@jmayor tests are failing with following error

gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
openrtb_ext/imp_datablocks.go

@jmayor requesting to address above comment

onkarvhanumante · 2023-07-24T07:53:07Z

@jmayor tests are failing with following error
gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
openrtb_ext/imp_datablocks.go
@jmayor requesting to address above comment

ping to make this change

onkarvhanumante · 2023-07-27T11:23:53Z

@jmayor tests on PR are failing with following error. Reminder to fix it so we can merge this PR

gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
openrtb_ext/imp_datablocks.go

jmayor · 2023-07-27T12:11:44Z

Sorry for the delay - I am on vacation in a very remote area with very little to no internet access. I will address this early next week when I am back

…

On Thu, Jul 27, 2023 at 7:24 AM Onkar Hanumante ***@***.***> wrote: @jmayor <https:/jmayor> tests on PR are failing with following error. Reminder to fix it so we can merge this PR gofmt needs to be run, 1 files have issues. Below is a list of files to review: openrtb_ext/imp_datablocks.go — Reply to this email directly, view it on GitHub <#2838 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AAEX5KGD2CZYPV6R5PJQZDLXSJFVJANCNFSM6AAAAAAZDVDAQI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

onkarvhanumante · 2023-08-11T09:26:37Z

Sorry for the delay - I am on vacation in a very remote area with very little to no internet access. I will address this early next week when I am back
…
On Thu, Jul 27, 2023 at 7:24 AM Onkar Hanumante @.> wrote: @jmayor https:/jmayor tests on PR are failing with following error. Reminder to fix it so we can merge this PR gofmt needs to be run, 1 files have issues. Below is a list of files to review: openrtb_ext/imp_datablocks.go — Reply to this email directly, view it on GitHub <#2838 (comment)>, or unsubscribe https:/notifications/unsubscribe-auth/AAEX5KGD2CZYPV6R5PJQZDLXSJFVJANCNFSM6AAAAAAZDVDAQI . You are receiving this because you were mentioned.Message ID: @.>

@jmayor ping to move this PR forward

guscarreon · 2023-08-22T17:16:33Z

The checks are failing because gofmt needs to be run. @jmayor friendly ping checking if you have time to move this PR forward.

jmayor · 2023-08-25T19:20:24Z

Appreciate the follow up and patience

I just pushed a fix

onkarvhanumante · 2023-08-30T09:13:30Z

@jmayor, jmayor:master feature branch is not in sync with prebid server master. Due to which Adapter code coverage and Adapter semgrep checks are failing.

Requesting to sync feature branch with updated master.

github-actions · 2023-08-30T11:42:25Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 34c7969

datablocks

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:22:	MakeRequests		80.0%
github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:71:	MakeBids		100.0%
github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:109:	splitImpressions	100.0%
github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:130:	getBidderParams		88.9%
github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:153:	getMediaType		100.0%
github.com/prebid/prebid-server/adapters/datablocks/datablocks.go:176:	Builder			100.0%
total:									(statements)		92.9%

jmayor · 2023-08-30T11:43:58Z

@onkarvhanumante Ok - looks like it passed all the tests now

onkarvhanumante · 2023-08-31T07:38:37Z

static/bidder-params/datablocks.json

- },
- "host": {
- "type": "string",
- "description": "Network Host to request from"
 }
 },
- "required": ["host", "sourceId"]
+ "required": ["sourceId"]


@jmayor this might be a breaking change. Should scenario where incoming auction request still has host. Prebid uses fail fast strategy. For example, if incoming auction request mentions multiple bidders. One of them is datablocks and has host param. Here request will fail in validation phase and entire auction will get aborted. This seems unfair for other bidders who are part of auction.

Therefore, consider having some transition period before removing host param. Giving enough opportunity to prebid server hosts/ publisher to make necessary change.

Recommended strategy is to not remove host param from the bidder-param and and required should only have sourceId. This will make host an optional param and can be removed after transition period

... "host": { "type": "string", "description": "Deprecated, Network Host to request from" } }, "required": ["sourceId"]

also should create docs pr to remove host param

There is no issue with removing a parameter. If a request is received with the old parameter, that value will simply be ignored and Prebid Server will be happy. This becomes a problem for adding a new required field.

@SyntaxNode that is what I assumed - we should be good to go

The host parameter is already listed as optional. This is ok.

I'm going to assume this PR will be merged soon enough that the datablocks adapter will be part of PBS 2.0. Thanks for the work @jmayor

@bretg Thanks for your patience

Update datablocks.yaml

e91920d

Remove dynamic domain with static domain

onkarvhanumante assigned onkarvhanumante and Sonali-More-Xandr Jun 13, 2023

onkarvhanumante reviewed Jun 13, 2023

View reviewed changes

Sonali-More-Xandr previously approved these changes Jun 13, 2023

View reviewed changes

Update datablocks_test.go

156e8a2

Update test url

jmayor dismissed Sonali-More-Xandr’s stale review via 156e8a2 July 14, 2023 20:42

updated server url and removed host variable requirement

a6d353d

jmayor added 2 commits August 25, 2023 15:17

remove tests that are no longer needed

3d67d88

remove host param

94c8279

Merge branch 'prebid:master' into master

34c7969

onkarvhanumante reviewed Aug 31, 2023

View reviewed changes

onkarvhanumante approved these changes Sep 1, 2023

View reviewed changes

bretg changed the title ~~Update datablocks.yaml~~ Update datablocks adapter Sep 1, 2023

Sonali-More-Xandr approved these changes Sep 4, 2023

View reviewed changes

Sonali-More-Xandr merged commit 47f840d into prebid:master Sep 4, 2023
5 checks passed

cross-repo-issue-creation bot mentioned this pull request Sep 4, 2023

Port PR from PBS-Go: Update datablocks adapter prebid/prebid-server-java#2604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update datablocks adapter #2838

Update datablocks adapter #2838

jmayor commented Jun 12, 2023

onkarvhanumante commented Jun 13, 2023

onkarvhanumante Jun 13, 2023

Sonali-More-Xandr commented Jun 21, 2023

onkarvhanumante commented Jun 30, 2023

onkarvhanumante commented Jul 14, 2023

jmayor commented Jul 14, 2023

Sonali-More-Xandr commented Jul 17, 2023

onkarvhanumante commented Jul 18, 2023

onkarvhanumante commented Jul 21, 2023

onkarvhanumante commented Jul 24, 2023

onkarvhanumante commented Jul 27, 2023

jmayor commented Jul 27, 2023 via email

onkarvhanumante commented Aug 11, 2023

guscarreon commented Aug 22, 2023

jmayor commented Aug 25, 2023

onkarvhanumante commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

jmayor commented Aug 30, 2023

onkarvhanumante Aug 31, 2023 •

edited

Loading

SyntaxNode Aug 31, 2023 •

edited

Loading

jmayor Aug 31, 2023

bretg Sep 1, 2023

jmayor Sep 1, 2023

		@@ -1,4 +1,4 @@
		endpoint: "http://{{.Host}}/openrtb2?sid={{.SourceId}}"
		endpoint: "http://pbserver.dblks.net/openrtb2?sid={{.SourceId}}"

Update datablocks adapter #2838

Update datablocks adapter #2838

Conversation

jmayor commented Jun 12, 2023

onkarvhanumante commented Jun 13, 2023

onkarvhanumante Jun 13, 2023

Choose a reason for hiding this comment

Sonali-More-Xandr commented Jun 21, 2023

onkarvhanumante commented Jun 30, 2023

onkarvhanumante commented Jul 14, 2023

jmayor commented Jul 14, 2023

Sonali-More-Xandr commented Jul 17, 2023

onkarvhanumante commented Jul 18, 2023

onkarvhanumante commented Jul 21, 2023

onkarvhanumante commented Jul 24, 2023

onkarvhanumante commented Jul 27, 2023

jmayor commented Jul 27, 2023 via email

onkarvhanumante commented Aug 11, 2023

guscarreon commented Aug 22, 2023

jmayor commented Aug 25, 2023

onkarvhanumante commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

Code coverage summary

datablocks

jmayor commented Aug 30, 2023

onkarvhanumante Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

SyntaxNode Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

jmayor Aug 31, 2023

Choose a reason for hiding this comment

bretg Sep 1, 2023

Choose a reason for hiding this comment

jmayor Sep 1, 2023

Choose a reason for hiding this comment

onkarvhanumante Aug 31, 2023 •

edited

Loading

SyntaxNode Aug 31, 2023 •

edited

Loading