[MAINTENANCE] Refactor legacy anonymous_usage_statistics into new top-level fields
#9891
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -57,7 +57,7 @@ def fixed_length_key(self): | |||
| def store_name(self): | |||
| return self._store_name | |||
|
|
|||
| def _construct_store_backend_id(self, suppress_warning: bool = False) -> Optional[str]: | |||
| def _construct_store_backend_id(self, suppress_warning: bool = False) -> Optional[uuid.UUID]: | |||
There was a problem hiding this comment.
Actively trying to fight against primitive obsession here - the data_context_id top level field has the opportunity to be a rich type instead of a string UUID
| @@ -1,7 +0,0 @@ | |||
| [ge_cloud_config] | |||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9891 +/- ##
===========================================
- Coverage 77.36% 77.33% -0.03%
===========================================
Files 493 493
Lines 42360 42238 -122
===========================================
- Hits 32770 32665 -105
+ Misses 9590 9573 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cdkini
changed the title
anonymous_usage_statistics into analytics and data_context_id top-level fieldsanonymous_usage_statistics into new top-level fields
| anonymous_usage_statistics: | ||
| enabled: false | ||
| data_context_id: b6743b23-ed35-4b09-814d-88ba86566295 | ||
| analytics: false |
There was a problem hiding this comment.
Looking at this now I feel like the name doesn't reflect that that it's a boolean. use_analytics maybe?
docs/docusaurus/static/_redirects
Outdated
| @@ -247,10 +247,6 @@ http://old.docs.greatexpectations.io/* http://648b46c2ec119e00089c4318--niobium- | |||
| /docs/reference/api/expectations/util.py /docs/reference/api/expectations/util | |||
| /docs/reference/api/render/util.py /docs/reference/api/render/util | |||
|
|
|||
| # Redirects for renamed reference docs | |||
|
|
|||
| /docs/reference/anonymous_usage_statistics /docs/reference/usage_statistics | |||
There was a problem hiding this comment.
My understanding is that we don't want to break links, and that this should redirect to the appropriate place in the legacy docs.
| # Analytics are enabled by default | ||
| # A user can opt out through the config or by setting the environment variable | ||
| # If one is set to False, analytics will be disabled | ||
| enabled_from_config = True if enabled_from_config is None else enabled_from_config |
There was a problem hiding this comment.
Why aren't we just defining the param as enabled_from_config: bool = True? Maybe I'm missing context, but why does it being enabled from config determine anything about analytics?
| @@ -297,11 +294,16 @@ def _init_factories(self) -> None: | |||
| ) | |||
|
|
|||
| def _init_analytics(self) -> None: | |||
| enabled_from_config = self.config.analytics | |||
| if enabled_from_config is None: | |||
There was a problem hiding this comment.
Same thoughts as above - I don't understand how enabled_from_config ties to analytics, and we should avoid None if we can just ensure it's instantiated with a default True
…_expectations into m/v1-158/analytics
…_expectations into m/v1-158/analytics
| usage_statistics_url: https://qa.stats.greatexpectations.io/great_expectations/v1/usage_statistics | ||
| enabled: false | ||
| data_context_id: f98647c3-0120-42ff-ae47-e48b8a6500cf | ||
| analytics: false |
| env_var_enabled = ENV_CONFIG.posthog_enabled | ||
|
|
||
| # A user must opt out through either the config file or env var to disable analytics | ||
| return config_file_enabled and env_var_enabled |
There was a problem hiding this comment.
This logic seems wrong (and doesn't match the comment). s/and/or. I might move the comment up to a docstring too!
…_expectations into m/v1-158/analytics
We're removing
anonymous_usage_statisticsin favor of two new top-level config options:analytics(a bool) anddata_context_id(an optional UUID)invoke lint(usesruff format+ruff check)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!