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

VIS.X Bid Adapter : add time tracking for HB response #10641

Closed

Conversation

vfedoseev
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

We have added the tracking of the HB response time for the VIS.X bidder adapter.

Other information

@vfedoseev
Copy link
Contributor Author

Hi all,

It seems that the job ci/circleci: e2etest was failed not because of the changes in this PR.

Thank you in advance for checking and have a nice day.

Best,
Vladimir

@ChrisHuie ChrisHuie changed the title VIS.X: add time tracking for HB response VIS.X Bid Adapter : add time tracking for HB response Oct 27, 2023
@ncolletti
Copy link
Contributor

changes look good to me. however, I haven't before seen an adapter firing a pixel for all bid responses, normally that will only occur on bid won events. @ChrisHuie could you confirm if this is OK or should it be discussed further?

@vfedoseev
Copy link
Contributor Author

Hi @ncolletti , @ChrisHuie ,

Pllease let us know if you should adjust anything in this PR or provide any additional information about the changes.

Thank you in advance.

Best,
Vladimir

Copy link
Contributor

@ncolletti ncolletti 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

@ncolletti
Copy link
Contributor

@ChrisHuie pr looks good. merge is blocked by a failed e2e test, can that be re-run?

Also, still curious about my question above regarding Prebid's stance on adapter modules firing on a interpretResponse method instead of the bidWon event

@yoc-rg
Copy link

yoc-rg commented Nov 16, 2023

Hey @ChrisHuie, could you please take a moment to review this PR? Please let us know if you need any clarification. Thank you!

@yoc-rg
Copy link

yoc-rg commented Nov 21, 2023

Hey @ncolletti, because Chris is not supporting us here, is there an option to assign this PR to another reviewer?

@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Nov 21, 2023

I can ask Patrick and Bret their thoughts on this today. Not sure if they will want to also bring up on the committee meeting after the holiday. I have seen this behavior only in analytics adapters not in a bid adapter.

@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Nov 21, 2023

@yoc-rg @vfedoseev Talked to the Prebid.js chairs. So a very similar pr was added to Kargo's Bid Adapter last year and we asked them to instead submit the change to their analytics adapter. This functionality feels like it should be in an Analytics Adapter.

#8510

@yoc-rg
Copy link

yoc-rg commented Nov 22, 2023

Hey @ChrisHuie, thank you so much for sharing these details, we'll take a look at the Kargo's adapters.

As an SSP, it's important for us to understand how much time it takes on the client side to send a request and download the response. To have a good measurement coverage, we decided to implement it in the interpretResponse method.
We currently don't have our analytical adapter, so creating one won't be the best way for us, it'd be hard to convince publishers to start using it.

Is using an analytical adapter the only way we can collect request/response time, or is there another way you could recommend?

@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Nov 29, 2023

Hey @yoc-rg (thanks for the new pr on this if an analytics adapter isn't possible)
So it is kind of a gray area for us right now since technically it isn't a rule to not have this type of thing in a bid adapter. We just frown upon it because we don't want it to become common practice in the ecosystem to add this into a lot of bid adapters over analytics adapters.

@ncolletti
Copy link
Contributor

#10772 was merged in, closing this PR

@ncolletti ncolletti closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants