Skip to content
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

Smaato: Send imp.ext object entirely #3995

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Enigo
Copy link
Contributor

@Enigo Enigo commented Oct 18, 2024

This PR refactors the way how our adapter sends the imp.ext object to the ad server - now the entire object is sent and not only skadn

Copy link

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, 4d58f1f

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/native.go:14:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:70:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:79:	MakeRequests			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:99:	MakeBids			85.3%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:163:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:196:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:226:	makePodRequests			84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:250:	makeRequest			85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:269:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:279:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:293:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:308:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:323:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:338:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:348:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:360:	setUser				95.2%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:403:	setExt				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:411:	setSite				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:431:	setApp				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:438:	setDOOH				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:445:	setPublisherId			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:468:	setImpForAdspace		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:487:	setImpForAdBreak		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:521:	removeBidderNodeFromImpExt	84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:543:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:563:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:583:	extractBidExt			100.0%
total:									(statements)			92.3%

@Enigo Enigo changed the title PREB-42 Add GPID support Smaato: Add GPID support Oct 18, 2024
@Enigo Enigo changed the title Smaato: Add GPID support Smaato: Send imp.ext object entirely Oct 18, 2024
@Enigo Enigo force-pushed the feature/PREB-42_O_add_gpid_support branch from 4d58f1f to ae02b61 Compare October 18, 2024 14:28
Copy link

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, ae02b61

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/native.go:14:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:70:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:79:	MakeRequests			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:99:	MakeBids			85.3%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:163:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:196:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:226:	makePodRequests			84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:250:	makeRequest			85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:269:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:279:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:293:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:308:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:323:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:338:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:348:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:360:	setUser				95.2%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:403:	setExt				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:411:	setSite				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:431:	setApp				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:438:	setDOOH				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:445:	setPublisherId			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:468:	setImpForAdspace		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:487:	setImpForAdBreak		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:521:	removeBidderNodeFromImpExt	84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:543:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:563:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:583:	extractBidExt			100.0%
total:									(statements)			92.3%

przemkaczmarek
przemkaczmarek previously approved these changes Oct 23, 2024
Copy link

@eros902002 eros902002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@bsardo
Copy link
Collaborator

bsardo commented Nov 4, 2024

Hi @Enigo, can you please merge with master (no rebase) and resolve conflicts? Thanks!

Copy link

github-actions bot commented Nov 5, 2024

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, 156b950

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/native.go:15:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:71:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:80:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:100:	MakeBids			85.3%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:164:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:197:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:227:	makePodRequests			84.6%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:251:	makeRequest			85.7%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:270:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:280:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:294:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:309:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:324:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:339:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:349:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:361:	setUser				95.2%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:404:	setExt				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:412:	setSite				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:432:	setApp				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:439:	setDOOH				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:446:	setPublisherId			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:469:	setImpForAdspace		80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:488:	setImpForAdBreak		89.5%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:522:	removeBidderNodeFromImpExt	84.6%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:544:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:564:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:584:	extractBidExt			100.0%
total:									(statements)			92.3%

@Enigo
Copy link
Contributor Author

Enigo commented Nov 5, 2024

@bsardo
done, please check

przemkaczmarek
przemkaczmarek previously approved these changes Nov 5, 2024
@Enigo
Copy link
Contributor Author

Enigo commented Nov 22, 2024

@bsardo
is it possible to get this PR approved and merged?

Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just one nitpick.

@@ -513,33 +514,33 @@ func setImpForAdBreak(imps []openrtb2.Imp) error {
imps[i].Video = &videoCopy
}

imps[0].Ext = impExt
imps[0].Ext = ext
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I think you can get rid of the ext variable and instead just assign firstImp.Ext to imps[0].Ext.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines +528 to +531
err := jsonparser.ObjectEach(updatedExt, func(key []byte, value []byte, dataType jsonparser.ValueType, offset int) error {
isEmpty = false
return nil
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is an interesting approach. I imagine this is much faster than unmarshaling to a map and checking the length.

Copy link

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, a10f684

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/native.go:15:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:71:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:80:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:100:	MakeBids			85.3%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:164:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:197:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:227:	makePodRequests			84.6%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:251:	makeRequest			85.7%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:270:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:280:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:294:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:309:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:324:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:339:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:349:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:361:	setUser				95.2%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:404:	setExt				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:412:	setSite				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:432:	setApp				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:439:	setDOOH				100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:446:	setPublisherId			100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:469:	setImpForAdspace		80.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:488:	setImpForAdBreak		88.9%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:521:	removeBidderNodeFromImpExt	84.6%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:544:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:564:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v3/adapters/smaato/smaato.go:584:	extractBidExt			100.0%
total:									(statements)			92.3%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants