Open kenzieschmoll opened 1 month ago
@andrewkolos can you work on this bug? It has pretty serious correctness implications for how we use analytics.
- User opens tool A and sees the analytics prompt. The user will be automatically opted into analytics collection for the second run of tool A.
- User decides to disable analytics by running the command. For the subsequent runs of tool A, analytics will not be collected.
- User opens tool B and sees the analytics prompt. User ignores prompt, and now the user will be opted into analytics again for the second run of tool B (and for usage of tool A since now analytics enabled = true).
Could you clarify what the expected behavior is here? My personal interpretation is that the user should be opted into telemetry for tool B but not for tool A, as these are two separate tools. However, the issue description seems to imply that disabling telemetry for tool A should disable telemetry for any other tool that is run in the future.
I do agree that tool A's telemetry should not be re-enabled without the user explicitly enabling it. This is definitely a bug.
I do agree that tool A's telemetry should not be re-enabled without the user explicitly enabling it. This is definitely a bug.
Actually, I didn't verify that this is even the case, so I am not even sure there is a bug here.
Could you clarify what the expected behavior is here? My personal interpretation is that the user should be opted into telemetry for tool B but not for tool A, as these are two separate tools. However, the issue description seems to imply that disabling telemetry for tool A should disable telemetry for any other tool that is run in the future.
The intent of unified_analytics is that there would be a single opt-in / opt-out state for analytics that is shared across all Dash tools. When we show the analytics notice, we also set the reporting flag to true, and then a user is expected to manually opt out if they do not want analytics to be collected. The bug is coming from when we decide to show the notice and set the opt-in to true.
As the PDD currently stands "If the reporting control flag ("reporting") is set to opt-out of reporting, then no notices should be shown in any tool." This is what we are currently violating by showing the analytics notice in tool B in the example above.
The intent of unified_analytics is that there would be a single opt-in / opt-out state for analytics that is shared across all Dash tools.
This was the part that surprised me to the point of me questioning whether I was reading this issue correctly. Thanks for clarifying. I'll get back to fixing this.
From reading the code and doing some testing on a fresh VM, I don't think the prompt on the second tool re-enables telemetry.
I wrote a test to verify this: https://github.com/dart-lang/tools/blob/5997ca9544c31e0b5b466ff89f832dcc1c5cd6d1/pkgs/unified_analytics/test/unified_analytics_test.dart#L1301-L1303
I also ported this test to the 5.8.8 branch on my local checkout, and it still passed.
So I think the only bug here is that Analytics::shouldShowMessage
will be true
whenever you are running some tool for the first time[^1], when it should only be true
the first time you are running any tool that uses unified_analytics.
[^1]: Or to be more technically precise, Analytics::clientShowedMessage
hasn't been called.
@andrewkolos to 1) take a second pass over the unified_analytics code to be confident that the bug described in this issue is not present and then 2) sync up with kenzieschmoll
@andrewkolos To 1) take a second pass over the unified_analytics code to be confident that the bug described in this issue is not present and then 2) sync up with kenzieschmoll
Andrew and I looked at the code, and we believe that:
flutter --enable-analytics
or dart --enable-analytics
etc.
Intended behavior per the PDD: "If the reporting control flag ("reporting") is set to opt-out of reporting, then no notices should be shown in any tool."
Current behavior:
The current logic sets
_showMessage
to true for the first run of a Dash tool: https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/analytics.dart#L447. When a message is shown, the tool showing the message will set analytics enabled to true.However, this doesn't take into account if the user has already run the command to disable analytics, which seems like a bug? Imagine this scenario:
This seems incorrect. If the user just opted out of analytics, we should not be prompting then again.
The code for
shouldShowMessage
just returns the value of_showMessage
, which doesn't take the enabled state into account: https://github.com/dart-lang/tools/blob/main/pkgs/unified_analytics/lib/src/analytics.dart#L516.Should this be modified to return
_showMessage && !_telemetrySuppressed
? Not sure whether we should check_telemetrySuppressed
or the value oftelemetryEnabled
, or both.@christopherfujino