Add metrics to determine the impact of AzureAD over-reporting [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
zm...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Owen Min
- Will Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Owen Min (Gerrit)
Owen Min voted Code-Review+1![]( "Open in Gerrit")
- Dmytro Yeroshkin
- Will Harris
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
BASE_EXPORT bool IsJoinedToAzureAD2();is the intent for all existing callers to migrate to this new API, and the old API to be deprecated?
I'm curious about the naming - because it seems to imply it behaves identically to `IsJoinedToAzureAD` but it doesn't...? What is the overall plan for these APIs going forward?
enum class AzureADJoinType {not exposed to non-test callers, I don't think this needs to be here, it can be inside the `ScopedAzureADJoinStateForTesting` class below.
Related details
- Dmytro Yeroshkin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmytro Yeroshkin (Gerrit)
Dmytro Yeroshkin added 2 comments![]( "Open in Gerrit")
BASE_EXPORT bool IsJoinedToAzureAD2();is the intent for all existing callers to migrate to this new API, and the old API to be deprecated?
I'm curious about the naming - because it seems to imply it behaves identically to `IsJoinedToAzureAD` but it doesn't...? What is the overall plan for these APIs going forward?
Yes, the plan is to migrate all existing call sites to use the new method in a follow-up CL (timing TBD).
not exposed to non-test callers, I don't think this needs to be here, it can be inside the `ScopedAzureADJoinStateForTesting` class below.
- Will Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 2 comments![]( "Open in Gerrit")
I don't really understand the purpose of this CL, if we just want to measure the metrics I don't think we need to add new public APIs. how will folks know which to call?
I'm not sure how the staging of this works... or what the metrics will be used for.
but I think I am happy to defer to owen here - I just worry we are adding a new public API here and how will people know whether or not to use it?
Can we defer adding the public API until we decide what the way forward is?
Related details
- Dmytro Yeroshkin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmytro Yeroshkin (Gerrit)
Dmytro Yeroshkin added 2 comments![]( "Open in Gerrit")
I don't really understand the purpose of this CL, if we just want to measure the metrics I don't think we need to add new public APIs. how will folks know which to call?
I'm not sure how the staging of this works... or what the metrics will be used for.
but I think I am happy to defer to owen here - I just worry we are adding a new public API here and how will people know whether or not to use it?
Can we defer adding the public API until we decide what the way forward is?
I'm not sure there is a clean way of adding these corrected metrics without making a function like this public in base/win_util.h. The results of this function need to be used in a different file+namespace. If you can suggest a clean way of doing this without a new public API, I'm definitely open to considering it.
I renamed the function to better reflect what it does to hopefully avoid confusion.
My understanding is that these metrics are of interest to the enterprise team to estimate the impact of removing the Azure AD over-reporting before we make the changes that will impact UMA and Omaha.
bool is_enterprise_device2 =Dmytro Yeroshkinnit: `const`
Done
Related details
- Will Harris
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
Dmytro YeroshkinI don't really understand the purpose of this CL, if we just want to measure the metrics I don't think we need to add new public APIs. how will folks know which to call?
I'm not sure how the staging of this works... or what the metrics will be used for.
but I think I am happy to defer to owen here - I just worry we are adding a new public API here and how will people know whether or not to use it?
Can we defer adding the public API until we decide what the way forward is?
I'm not sure there is a clean way of adding these corrected metrics without making a function like this public in base/win_util.h. The results of this function need to be used in a different file+namespace. If you can suggest a clean way of doing this without a new public API, I'm definitely open to considering it.
I renamed the function to better reflect what it does to hopefully avoid confusion.
My understanding is that these metrics are of interest to the enterprise team to estimate the impact of removing the Azure AD over-reporting before we make the changes that will impact UMA and Omaha.
I'm sorry as a base/win owner I don't agree with adding a new api in base for just collecting the metrics
I think if you need to collect the metrics this can be done internally to `components/policy/core/common/policy_loader_win.cc` maybe?
I am still not sure what these metrics will be used for to be honest. Perhaps Owen can comment?
I think once we have metrics we should then have one unified API that behaves correctly according to the behavior that we desire.
in my mind we are simply fixing a bug here - it was never intended that `IsJoinedToAzureAD` should return true if the device is not joined to azure ad, so I would just fix the bug (you could add default-enabled killswitch if you think this will cause issues)
If you want to be more cautious than this, collect the metrics internally in components/policy but changing the public API before we know what we are doing seems wrong.
Related details
- Dmytro Yeroshkin
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmytro Yeroshkin (Gerrit)
Dmytro Yeroshkin added 1 comment![]( "Open in Gerrit")
Dmytro YeroshkinI don't really understand the purpose of this CL, if we just want to measure the metrics I don't think we need to add new public APIs. how will folks know which to call?
I'm not sure how the staging of this works... or what the metrics will be used for.
but I think I am happy to defer to owen here - I just worry we are adding a new public API here and how will people know whether or not to use it?
Can we defer adding the public API until we decide what the way forward is?
Will HarrisI'm not sure there is a clean way of adding these corrected metrics without making a function like this public in base/win_util.h. The results of this function need to be used in a different file+namespace. If you can suggest a clean way of doing this without a new public API, I'm definitely open to considering it.
I renamed the function to better reflect what it does to hopefully avoid confusion.
My understanding is that these metrics are of interest to the enterprise team to estimate the impact of removing the Azure AD over-reporting before we make the changes that will impact UMA and Omaha.
I'm sorry as a base/win owner I don't agree with adding a new api in base for just collecting the metrics
I think if you need to collect the metrics this can be done internally to `components/policy/core/common/policy_loader_win.cc` maybe?
I am still not sure what these metrics will be used for to be honest. Perhaps Owen can comment?
I think once we have metrics we should then have one unified API that behaves correctly according to the behavior that we desire.
in my mind we are simply fixing a bug here - it was never intended that `IsJoinedToAzureAD` should return true if the device is not joined to azure ad, so I would just fix the bug (you could add default-enabled killswitch if you think this will cause issues)
If you want to be more cautious than this, collect the metrics internally in components/policy but changing the public API before we know what we are doing seems wrong.
Current proposal is to follow this with a CL that disables sensitive policy enforcement for devices with workspace joined state. And then switch other places that use the existing logic 2 milestones later.
This means that in the short term, we need to be able to distinguish between the 3 possible Azure AD states: Not joined, device joined, workspace joined. The only place to obtain this data is the win32 API that we currently access from `GetAzureADJoinStateStorage` in win_util.cc by loading a dll. As far as I can tell, this means we need to either expose a new method, modify `IsJoinedToAzureAD` to return an enum, or load the dll in yet another file.
Out of those 3 options the last one seems the most problematic to me. In the current version of the CL I went with option 1, but option 2 (or some variant thereof) is perfectly as acceptable to me.
As for why we want to collect these metrics, one of the key items from my understanding is that same logic that controls these metrics, also impacts Omaha enterprise vs consumer metrics, so we want to gauge the impact before we shift Omaha DAU by a potentially significant margin. I will also ping Owen in chat to get him to weight in on this.
Related details
- Owen Min
- Will Harris
Owen Min (Gerrit)
Owen Min voted and added 1 comment![]( "Open in Gerrit")
Votes added by Owen Min
| Code-Review | +1 |
Dmytro YeroshkinI don't really understand the purpose of this CL, if we just want to measure the metrics I don't think we need to add new public APIs. how will folks know which to call?
I'm not sure how the staging of this works... or what the metrics will be used for.
but I think I am happy to defer to owen here - I just worry we are adding a new public API here and how will people know whether or not to use it?
Can we defer adding the public API until we decide what the way forward is?
Will HarrisI'm not sure there is a clean way of adding these corrected metrics without making a function like this public in base/win_util.h. The results of this function need to be used in a different file+namespace. If you can suggest a clean way of doing this without a new public API, I'm definitely open to considering it.
I renamed the function to better reflect what it does to hopefully avoid confusion.
My understanding is that these metrics are of interest to the enterprise team to estimate the impact of removing the Azure AD over-reporting before we make the changes that will impact UMA and Omaha.
Dmytro YeroshkinI'm sorry as a base/win owner I don't agree with adding a new api in base for just collecting the metrics
I think if you need to collect the metrics this can be done internally to `components/policy/core/common/policy_loader_win.cc` maybe?
I am still not sure what these metrics will be used for to be honest. Perhaps Owen can comment?
I think once we have metrics we should then have one unified API that behaves correctly according to the behavior that we desire.
in my mind we are simply fixing a bug here - it was never intended that `IsJoinedToAzureAD` should return true if the device is not joined to azure ad, so I would just fix the bug (you could add default-enabled killswitch if you think this will cause issues)
If you want to be more cautious than this, collect the metrics internally in components/policy but changing the public API before we know what we are doing seems wrong.
Current proposal is to follow this with a CL that disables sensitive policy enforcement for devices with workspace joined state. And then switch other places that use the existing logic 2 milestones later.
This means that in the short term, we need to be able to distinguish between the 3 possible Azure AD states: Not joined, device joined, workspace joined. The only place to obtain this data is the win32 API that we currently access from `GetAzureADJoinStateStorage` in win_util.cc by loading a dll. As far as I can tell, this means we need to either expose a new method, modify `IsJoinedToAzureAD` to return an enum, or load the dll in yet another file.
Out of those 3 options the last one seems the most problematic to me. In the current version of the CL I went with option 1, but option 2 (or some variant thereof) is perfectly as acceptable to me.
As for why we want to collect these metrics, one of the key items from my understanding is that same logic that controls these metrics, also impacts Omaha enterprise vs consumer metrics, so we want to gauge the impact before we shift Omaha DAU by a potentially significant margin. I will also ping Owen in chat to get him to weight in on this.
### Metrics
We need metrics as there are many features depends on the device management state, in Chrome, Google Update and Remote desktop.
See **Impact** section in https://docs.google.com/document/d/13kNPlcHqxSpb01KfCzlAK1Uw_3ru2lnfIX9I4yYeO-0/edit?resourcekey=0-3Cu1sQcUF4RNPuoT_WRGDg&tab=t.0#heading=h.e9cg4xchba6v for all affected area.
So we need to be careful about the fix and the first step is using metrics to understand the scale of affected users.
### Two APIs
Depends on the outcome of metrics, we may take different approach. If the number is big, we may have to have a slow migration. Sensitive policies check will be the first one but others may have to wait a bit so that enterprise can have time to prepare for the change. What happened in the past is enterprises users workflows depend on the bug we have and were very upset when we fixed them.
It means we will need to support both old and new impl of `IsJoinedToAzureAD` for a few milestones.
Related details
- Dmytro Yeroshkin
- Will Harris