-
Notifications
You must be signed in to change notification settings - Fork 173
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
Add Greenbids Analytics Adapter #3096
Add Greenbids Analytics Adapter #3096
Conversation
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/CommonMessage.java
Outdated
Show resolved
Hide resolved
this.auctionId = auctionContext.getBidRequest().getId(); | ||
this.referrer = auctionContext.getBidRequest().getSite().getPage(); | ||
this.sampling = greenbidsConfig.getGreenbidsSampling(); | ||
this.prebid = "$prebid.version$"; // TODO: to fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review: currently searching where PBS version field could be extracted from to the payload
.filter(seatNonBid -> !seatNonBid.getNonBid().isEmpty()) | ||
.toList(); | ||
|
||
final Map<String, NonBid> seatsWithNonBids = Optional.of(seatNonBids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review: When I’ve been testing the AuctionContext.bidRejectionTrackers sending the request for storedresponse for the Rubicon bidder I get both
seatsWithNonBids
:{rubicon=NonBid(impId=a, statusCode=NO_BID)}
extracted fromAuctionContext.bidRejectionTrackers
seatsWithBids
:{rubicon=Bid(id=1, impid=a, price=1.23)}
extracted fromAuctionContext.BidResponse.seatBid
is it possible to have 2 responses for the one ad unit from the same bidder in both seat no bid and seat bid or maybe I’m missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the comments first, then I'll continue with the review
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
long hashInt = Math.abs(greenbidsId.hashCode()); | ||
hashInt = hashInt % 0x10000; | ||
|
||
final boolean isPrimarySampled = hashInt < exploratorySamplingRate * 0xFFFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not plain integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we apply here the hexadecimal as we use it on the side of Greenbids analytics server. In this case we ensure that on the given auction we apply the hash function is applied similarly on both PBS and analytics server. And we use the same approach in Prebid.js analytics adapter
def exploration_value(self):
hash_value = int(greenbidsId[-4:], 16)
max_value = 0xFFFF + 1
return hash_value / max_value
and then compare it with the exploration_value < exploration_rate or exploration_value > (1 - exploration_rate)
final double throttledSamplingRate = samplingRate * (1.0 - this.exploratorySamplingSplit); | ||
|
||
long hashInt = Math.abs(greenbidsId.hashCode()); | ||
hashInt = hashInt % 0x10000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not plain integer? why is it calculated in two steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning the conversion to hexadecimal similar as in previous comment.
Concerning two steps yes I agree, I've modified Math.abs(greenbidsId.hashCode()) % 0x10000
in a one step
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
82f2cf9
to
b13883c
Compare
Hi the fixes are made, @AntoxaAntoxic could you please have a look 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next package of comments
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/HttpUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/JsonUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/spring/config/AnalyticsConfiguration.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAuctionContext.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
public Future<CommonMessage> createBidMessage( | ||
AuctionContext auctionContext, | ||
BidResponse bidResponse, | ||
String greenbidsId, | ||
String billingId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, it's no necessary that each method returns future
also why is the method public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have reverted changes for createBidMessage
returning futures.
I made this method public because I then call it in unit test GreenbidsAnalyticsReporterTest
. This is because in unit test I check the content of the payload without sending it to analytics server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise if I make the method createBidMessage
private then in UT I will need to call processEvent
instead and mock both creation of a payload and send to the mocked server.
The analytics server could be mocked using PowerMock but in PBS I did not find that it was used before
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
@JsonProperty("version") | ||
private final String version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not necessary to annotate if the field name is a single lowercased word otherwise it's better to check whether our jackson mapper serialize in a way you require
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
final ObjectMapper mapper = Objects.requireNonNull(jacksonMapper).mapper() | ||
.setPropertyNamingStrategy(PropertyNamingStrategy.LOWER_CAMEL_CASE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the problem in adding @JsonProperty annotation, but if you really want your own mapper then please create it separately without changing the existing one, in that way you won't be dependent on the changes that might be done on the higher level that could bring backward compatibility problems in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think I can just create a custom JacksonMapper specifically for our module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have defined custom mapper GreenbidsJacksonMapper
and set naming strategy lower camel case into its ObjectMapper
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/spring/config/AnalyticsConfiguration.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Show resolved
Hide resolved
src/main/java/org/prebid/server/spring/config/AnalyticsConfiguration.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
...test/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/MediaTypes.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/GreenbidsUnifiedCode.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/GreenbidsPrebidExt.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/GreenbidsAdUnit.java
Outdated
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/GreenbidsAnalyticsReporter.java
Show resolved
Hide resolved
src/main/java/org/prebid/server/analytics/reporter/greenbids/model/GreenbidsJacksonMapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been a long journey, but now it is over. ✅
LGTM 👍
We want to add the Greenbids Analytics Adapter to transfer the payload to Greenbids analytics server
a.greenbids.ai
according to the following schema. The payload is gathered when invoking the auction context from the PBS endpointsopenrtb2/auction
andopenrtb2/amp
.The example of the test payload of the given adapter is as follows