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 availability per model line for DataProvider and EventGroup #221

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stevenwarejones
Copy link
Contributor

Addresses #220

@wfa-reviewable
Copy link

This change is Reviewable

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

  google.type.Interval data_availability_interval = 11 [deprecated = true];

  message DataAvailability {

Maybe we don't need per model line availability at the Event Group level since the EG is always constrained to be a subset of the DP availability.

At least, I can't think of a case where it would help and having this level of granularity seriously complicates things.

Maybe you have a case in mind?

Copy link
Contributor Author

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Maybe we don't need per model line availability at the Event Group level since the EG is always constrained to be a subset of the DP availability.

At least, I can't think of a case where it would help and having this level of granularity seriously complicates things.

Maybe you have a case in mind?

It is completely needed if EDPs don't label all campaigns with all available VID Models on a given day. In that case, they can't even populate the DataProvider fields at all. Maybe the DataProvider availability should be optional in that case.

I see this scenario playing out if EDPs always label a campaign with whatever model line they first started labeling the campaign.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

It is completely needed if EDPs don't label all campaigns with all available VID Models on a given day. In that case, they can't even populate the DataProvider fields at all. Maybe the DataProvider availability should be optional in that case.

I see this scenario playing out if EDPs always label a campaign with whatever model line they first started labeling the campaign.

That's pretty hypothetical and adds a lot of additional complexity.

It could always be added in the future if/when we encounter the scenario you mention. We could even oneof it with the global data availability field and maintain backward compatibility.

Copy link
Contributor Author

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

That's pretty hypothetical and adds a lot of additional complexity.

It could always be added in the future if/when we encounter the scenario you mention. We could even oneof it with the global data availability field and maintain backward compatibility.

Its very real. We've talked about that scenario. My takeaway is that each market (or possibly globally) will either go in the direction of needing it on Event Groups or on the Data Provider but probably not both (per market).

Maybe we should wait until there's direction on that.

Or it could evolve with the DataProvider one being needed and then the market evolves to needing it on the Event Group level.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

Its very real. We've talked about that scenario. My takeaway is that each market (or possibly globally) will either go in the direction of needing it on Event Groups or on the Data Provider but probably not both (per market).

Maybe we should wait until there's direction on that.

Or it could evolve with the DataProvider one being needed and then the market evolves to needing it on the Event Group level.

So we did talk about the scenario of continuing to label a given EG with the same model line for the life time of the EG, but I don't think that's the right way to approach labeling because in general that will not allow you to meaningfully report across EGs.

However, even if it were desirable I'm not convinced that it requires this field.

I see some options for moving forward. In my order of preference:

  1. Hold on all of this until we have some guidance from the model provider about how they want to manage VID model life times.
  2. Write a doc that describes the cases that require this and demonstrate the need. I started to work an example and I don't think these fields are necessary, but I didn't have time to work it all the way through. And there may be other cases.
  3. Drop the fields from the PR and we can deal with adding them when we have an identified need.

Copy link
Contributor Author

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

So we did talk about the scenario of continuing to label a given EG with the same model line for the life time of the EG, but I don't think that's the right way to approach labeling because in general that will not allow you to meaningfully report across EGs.

However, even if it were desirable I'm not convinced that it requires this field.

I see some options for moving forward. In my order of preference:

  1. Hold on all of this until we have some guidance from the model provider about how they want to manage VID model life times.
  2. Write a doc that describes the cases that require this and demonstrate the need. I started to work an example and I don't think these fields are necessary, but I didn't have time to work it all the way through. And there may be other cases.
  3. Drop the fields from the PR and we can deal with adding them when we have an identified need.

I prefer option 3. Only have the dataprovider fields. The current main behavior is hard to reason what to do once you add a new modelline. Of course, there are no model lines to report on.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/event_group.proto line 135 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I prefer option 3. Only have the dataprovider fields. The current main behavior is hard to reason what to do once you add a new modelline. Of course, there are no model lines to report on.

We can add the fields, but because we need these fields to have referential integrity, we can't use them until Origin starts populating the Model Line resource.

You might wonder why I wouldn't hold the same standard for the model line field in the Measurement Spec, and it's because Measurements have a retention policy and are essentially temporary objects, whereas the Data Provider and Event Group objects are durable.

So let's say we add the DP fields now. What's the plan going forward for them? How do they get adopted?

Maybe we can discuss on Monday?

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.

3 participants