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

Checking for devtools config file for opt out #227

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

eliasyishak
Copy link
Contributor

This PR will make sure that when the config file for package:unified_analytics is being created the first time, it will check in the devtools config file for previous opt out status. If the user has already opted out in the past but haven't interacted with package:unified_analytics yet, then it will pass that information and opt out the user from package:unified_analytics collection.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@@ -137,13 +137,17 @@ Directory? getHomeDirectory(FileSystem fs) {
/// Dart: `$HOME/.dart/dartdev.json`
///
/// Flutter: `$HOME/.flutter`
///
/// Devtools: `$HOME/.flutter-devtools/.devtools`
Copy link
Member

Choose a reason for hiding this comment

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

So here "legacy" doesn't necessarily refer to the analytics instance it's going to, but an implementation of analytics that predates package:unified_analytics, right?

And therefore, we will need to maintain this code in perpetuity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't it mean both? Currently devtools has their own implementation for analytics while it also sends to a different GA instance.. is that what you mean?

And yes, we would maintain this. It only runs the first time package:unified_analytics is run on a developers workstation though

@christopherfujino
Copy link
Member

christopherfujino commented Jan 24, 2024

This looks good, but I'll defer approval to Kenzie

// reason, the file was written incorrectly
return true;
} on FileSystemException {
return true;
Copy link
Contributor

@kenzieschmoll kenzieschmoll Jan 24, 2024

Choose a reason for hiding this comment

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

i see this is the same logic for other tools with legacy opt in / opt out files, but why assume they are opted out if we can't read the file? Seems like instead we should assume that they haven't answered the question as to whether they want to opt in or not, and then we can ask them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair question, I think we wanted to approach this with a more conservative approach. I was imagining a developer who thought they were opted out in the past, somehow ended up with a corrupted file, and now gets prompted again... they may dismiss it thinking it was bug and now we're collecting their data

I'm not opposed to swapping out this logic, just wanted to highlight that edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that if a user opted out once, they'd opt out again when prompted. The user set I'd be worried about dropping are the users who have opted yes, and then somehow their file gets corrected, and now we opt them out instead of asking them again, which would show up as a drop in our usage that could be due to the logic here.

I would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.

I have a separate PR that is out that will allow us to track how many times we run into these catch blocks so that should help answer if this happens often in the wild

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 5.8.1 ready to publish unified_analytics-v5.8.1
package:cli_config 0.1.2 already published at pub.dev
package:extension_discovery 2.0.1-wip WIP (no publish necessary)
package:graphs 2.3.2-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@eliasyishak eliasyishak merged commit b97bd5c into dart-lang:main Jan 29, 2024
6 checks passed
@eliasyishak eliasyishak deleted the check-devtools-for-opt-out branch January 29, 2024 18:04
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 30, 2024
… mime, mockito, test, tools, vector_math

Revisions updated by `dart tools/rev_sdk_deps.dart`.

args (https://github.com/dart-lang/args/compare/46d5033..03386ba):
  03386ba  2024-01-23  Albert Kaiser  Add missing curly braces in README.md (dart-lang/args#261)

csslib (https://github.com/dart-lang/csslib/compare/1ad2d1e..ec86ee5):
  ec86ee5  2024-01-09  Kevin Moore  Require Dart 3.0, update and fix lints (dart-lang/csslib#194)

ecosystem (https://github.com/dart-lang/ecosystem/compare/1e2785d..9ee08a4):
  9ee08a4  2024-01-29  Moritz  Add `ignore` flag to health workflows (dart-lang/ecosystem#218)
  a283d70  2024-01-17  Moritz  Make health testable (dart-lang/ecosystem#224)
  f61a550  2024-01-16  Moritz  Enable experiments for health (dart-lang/ecosystem#226)
  c81f25c  2024-01-16  Moritz  Add submodule support to `publish.yaml` (dart-lang/ecosystem#225)
  b51c356  2024-01-12  Moritz  Add submodules support to `health.yaml` (dart-lang/ecosystem#223)
  d7aaecb  2024-01-10  Moritz  Run `health.yaml` for bots (dart-lang/ecosystem#222)
  971c733  2024-01-10  Moritz  Don't write the failure string when skipping (dart-lang/ecosystem#220)

html (https://github.com/dart-lang/html/compare/06bc148..910f6d7):
  910f6d7  2024-01-26  Kevin Moore  Update lints, require Dart 3.2 (dart-lang/html#236)
  aaf7d1a  2024-01-25  Kevin Moore  blast_repo fixes (dart-lang/html#235)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/ae48489..491f7c6):
  491f7c6  2024-01-24  Kevin Moore  Update lints, require Dart 3.2 (dart-lang/http_multi_server#63)
  0df95e0  2024-01-24  Kevin Moore  blast_repo fixes (dart-lang/http_multi_server#62)

logging (https://github.com/dart-lang/logging/compare/4d35a4e..e04942d):
  e04942d  2024-01-18  Kevin Moore  update min SDK and deps (dart-lang/logging#155)
  a03a946  2024-01-18  Craig Labenz  Hierarchical logging documentation (dart-lang/logging#146)
  439ec80  2024-01-18  Kevin Moore  blast_repo fixes (dart-lang/logging#154)

mime (https://github.com/dart-lang/mime/compare/ca9f059..99fbdcc):
  99fbdcc  2024-01-24  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-lang/mime#114)

mockito (https://github.com/dart-lang/mockito/compare/e15e000..0422551):
  0422551  2024-01-10  Oleh Prypin  Ignore "must_be_immutable" warning in generated files.

test (https://github.com/dart-lang/test/compare/846d73e..6700049):
  6700049d  2024-01-29  Nate Bosch  Prepare to publish package:checks (dart-lang/test#2178)
  a5c4f010  2024-01-24  Nate Bosch  Use a raw string for console logging with path (dart-lang/test#2177)
  fe3102ee  2024-01-10  dependabot[bot]  Bump js from 0.6.7 to 0.7.0 in /pkgs/test (dart-lang/test#2168)
  c709cde0  2024-01-10  Jacob MacDonald  fix a bug where test html files were not created in precompiled mode (dart-lang/test#2170)
  0eddae47  2024-01-09  Nate Bosch  Document the silent reporter (dart-lang/test#2163)

tools (https://github.com/dart-lang/tools/compare/8ffc077..f6e67f2):
  f6e67f2  2024-01-29  Elias Yishak  Fix logic for first run to fix `shouldShowMessage` behavior (dart-lang/tools#228)
  b97bd5c  2024-01-29  Elias Yishak  Checking for devtools config file for opt out (dart-lang/tools#227)

vector_math (https://github.com/google/vector_math.dart/compare/38a00c3..cb976c7):
  cb976c7  2024-01-28  Andrew Brampton  Update README.md to show how to use vector_math_64. (google/vector_math.dart#312)
  d99c903  2024-01-25  Andrew Brampton  Added a toString, operator == and hashCode to the Quad class. (google/vector_math.dart#311)

Change-Id: Ie42ec078b7b4d408d5167e38f05f1f37b754afb0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/349301
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
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.

3 participants