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

Add instance level enable_events config #2479

Closed
shahinrahbariasl opened this issue Dec 5, 2022 · 6 comments
Closed

Add instance level enable_events config #2479

shahinrahbariasl opened this issue Dec 5, 2022 · 6 comments
Assignees
Labels

Comments

@shahinrahbariasl
Copy link
Contributor

shahinrahbariasl commented Dec 5, 2022

Currently events such as imp and win can be enabled in Prebid Server using stored accounts (by setting enable_events) on accounts level. This requires a new stored account to be created for every new account. However we can move enable_events config to instance level. That way if hosting PBS, events tracking can be enabled/disabled for all accounts.

More info:
PBS event handler checks for events being enabled:

if !account.EventsEnabled {

@bretg
Copy link
Contributor

bretg commented Dec 22, 2022

@shahinrahbariasl - I'm ok with a host-level flag, but it would be overridden by account-level config. The order of precedence we use in PBS is

  1. request-level params
  2. account-level config
  3. host-level config
  4. hardcoded defaults

If you want to deviate from this convention for this use case, please explain the scenario

@bretg bretg moved this from Triage to Clarify Request in Prebid Server Prioritization Dec 22, 2022
@ShriprasadM
Copy link
Contributor

ShriprasadM commented Dec 24, 2022

@bretg we are working on #1725. As a part of this thing we have introduced events.enabled flag and planning to deprecate accounts.events_enabled flag (As per #3 - https://docs.google.com/document/d/1tH6I9Vho-_KaUIGNCMum9JwbxHDc0FyG2wD8T-h3wkY/edit?usp=sharing mentioned here))

@shahinrahbariasl
Copy link
Contributor Author

shahinrahbariasl commented Dec 29, 2022

Hi @bretg and @ShriprasadM , I was thinking more like the other way which would be instance level flag taking precedence since the use case I'm trying to tackle is, when hosting PBS, we would want to enable events for all accounts without the need to update the existing stored accounts or create new ones. I had a draft PR which I just mentioned in this issue. The existing logic only checks for account.EventsEnabled but the logic I was thinking was more like:

if !account.EventsEnabled && !e.Cfg.Event.Enabled {
		...
                // not enabled
		return
	}

If we want to make stored accounts override instance level, the current config is enable_event so I think that should be negated to have a config that disables events but that's a bit out of the scope I was hoping for this change and also it would require changing the existing stored accounts. Or we could add a new field to stored account configs to disable events? but again, that'd be confusing with the existing enable_event account config.

With the above logic, in case someone needs to disable events for an account, they can then remove the instance level config and manage the events per account with stored accounts.

Let me know what you think or if I'm missing something.

@shahinrahbariasl
Copy link
Contributor Author

shahinrahbariasl commented Jan 9, 2023

Ok was able to make stored account override instance level by changing stored account event flag to *bool instead of bool so that we can distinguish between nil and false values: #2509

@bsardo
Copy link
Collaborator

bsardo commented Feb 23, 2023

@ShriprasadM I see you referenced #1725 above and I originally thought this issue and PR #2533 were related to that but it appears you're just looking to make a stopgap change by making an adjustment to a field (accounts.events_enabled) you intend to deprecate as part of the #1725 plan, is that correct?

I got confused; ignore the above.

@bretg bretg moved this from Clarify Request to Ready for Dev in Prebid Server Prioritization Mar 24, 2023
@bsardo bsardo added the PBS-Go label Mar 29, 2023
@bsardo
Copy link
Collaborator

bsardo commented Dec 15, 2023

Closing. This is complete. Instead of an events_enabled config flag at the host level, we have an events.enabled flag in the host account defaults as well as on each account.

@bsardo bsardo closed this as completed Dec 15, 2023
@github-project-automation github-project-automation bot moved this from Ready for Dev to Done in Prebid Server Prioritization Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

4 participants