New Analytics Adapter: agma #3400
Conversation
For the record, it's ok to have a host configuration that allows the host company to decide to skip the Purpose 7 check. |
|
Thanks for reviewing this, @bsardo. Could you give me a rough estimate of when you will have time for this? |
steffenmllr
force-pushed
the
feature/agma-analytics
branch
from
16e26df
to
0950638
Compare
|
@SyntaxNode - Is there anything else you want me to change? |
|
@bsardo // @AlexBVolcy // @bretg - Could you please provide me with an estimate of when this can be merged? |
|
@steffenmllr I apologize for not responding as I didn't realize you kept pinging me. I was holding off on this because I wanted to get the TCF purpose 7 logic into PBS core first. I created a PR for it last year but hadn't had a chance to revisit it. I'll try to get to this and put up my purpose 7 PR this week. |
|
@bsardo - thanks for the info! You are taking about this? #2823. This would remove the need for me to manually check the consent like this: https://github.com/prebid/prebid-server/pull/3400/files#diff-f65aea57707f44f34323f70c397368ab5e22e7f2cb6273e1d6f1fc750c382d1eR165-R183 If there is anything I can do or help/review, please let me know! |
|
@bsardo - I apologize if I seem overly persistent; I was wondering if there might be any updates you could share regarding the current status? If you give me a list of do's for #2823, I can take a look at it and try to push this forward. From my understanding, this ports the Java functionality, right? Thank you very much for your time and consideration. |
|
@bsardo, I also wanted to ask if it would be an option to review & merge this PR first and replace the purpose 7 handling when you have the time, since our adapter handles p7 by itself. |
|
After a discussion with our privacy lawyer, we decided that in our case (aggregated market research) it's Purpose 9 that applies here rather than Purpose 7. I've updated the code to reflect this. Would be great if you could review this, @bsardo - a lot of technology partners in Germany are waiting for this PR so they can start rolling it out. |
analytics/agma/agma_module.go
Outdated
| if err != nil { | ||
| l.reset() | ||
| l.mux.Unlock() | ||
| glog.Warning("[AgmaAnalytics] fail to close the json array") |
There was a problem hiding this comment.
Is there a way to cover these error lines?
There was a problem hiding this comment.
I think l.buffer.Write can only fail if l.buffer is nil (which can not be the case) or the expanding of the buffer with the tryGrowByReslice on the buffer itself. Since we also don't check the l.buffer.Write I think would would like to remove this error check since it does reset on the buffer a couple lines down on error - what do you think
| if err != nil { | ||
| l.reset() | ||
| l.mux.Unlock() | ||
| glog.Warning("[AgmaAnalytics] fail to copy the buffer") | ||
| return |
There was a problem hiding this comment.
Is there a way to cover these error lines?
There was a problem hiding this comment.
Good question. I believe this situation can only occur if the buffer is empty and does not reset. However, this would effectively test the buffer's Read functionality. Do you have a specific test case in mind?
There was a problem hiding this comment.
I think it's okay to leave it uncovered.
| } | ||
|
|
||
| if publisherId == "" && appSiteId == "" { | ||
| return false, "" |
There was a problem hiding this comment.
Can you cover this line?
| l.mux.Lock() | ||
|
|
||
| if l.eventCount == 0 || l.buffer.Len() == 0 { | ||
| l.mux.Unlock() |
There was a problem hiding this comment.
I wonder if you can remove all of the Unlock calls, and instead utilize a defer l.mux.Unlock() instead to ensure it's calling when the function returns. What do you think?
There was a problem hiding this comment.
This was the initial design i had in the first Adapter, as per #3299 (comment) - which this PR is based of - I'm handling this explicitly
There was a problem hiding this comment.
I think there may be another solution to the issue:
defer {
l.reset()
l.mux.Unlock() }
This would prevent the reset and unlock becoming detached from each other. But in this case you don't want this pattern, as you have a path that doesn't call l.reset().
analytics/agma/agma_module.go
Outdated
| if requestWrapper.Site != nil { | ||
| if requestWrapper.Site.Publisher != nil { | ||
| publisherId = requestWrapper.Site.Publisher.ID | ||
| } | ||
| appSiteId = requestWrapper.Site.ID | ||
| } | ||
| if requestWrapper.App != nil { | ||
| if requestWrapper.App.Publisher != nil { | ||
| publisherId = requestWrapper.App.Publisher.ID | ||
| } | ||
| appSiteId = requestWrapper.App.ID | ||
| } |
There was a problem hiding this comment.
Could you extract this code that seems repetitive to its own function?
| if resp.StatusCode != http.StatusOK { | ||
| glog.Errorf("[agmaAnalytics] Wrong code received %d instead of %d", resp.StatusCode, http.StatusOK) | ||
| return fmt.Errorf("wrong code received %d instead of %d", resp.StatusCode, http.StatusOK) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Is there a way to cover/trigger these lines with tests.
|
@hhhjort // @AlexBVolcy - Thanks for review! If you need me to change anything else please let me know.
|
|
Thanks for the approval. Any chance this can make it in the next release? |
|
@steffenmllr - do you need this ported to PBS-Java? |
|
@bretg yes it would be great to also have this in prebid java, we just wanted to focus on golang first. Would this be handled as the prebid bidders or should we add a PR ? |
|
@steffenmllr - it will eventually get ported by the team, but we don't commit to a specific timeline. Depends on how busy we are with features and the size of the backlog. If there's a specific date you need the port, you should consider doing it yourself. If you're flexible, then we'll get to it. |
Following up on #3299 (comment), this is a refactored version of the HTTP analytics PR, scoped to the data used by agma.
As requested by @bretg, I've limited the data collection to our GVLID and Propose 7.
Since I'm not entirely familiar with the entire codebase, I have some questions regarding whether there is a better way to do things:
consentin theuserExt, as this was the only place I found the user consent in my environment. Is this the correct approach, or is the consent also available in the OpenRTB user object?Thank you for your review.