Page MenuHomePhabricator

Change telemetry to be opt-in instead
ClosedPublic

Authored by catherinewu on Wed, Mar 18, 10:39 PM.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

catherinewu created this revision.Wed, Mar 18, 10:39 PM
alangenfeld added inline comments.Thu, Mar 19, 12:03 AM
python_modules/dagster/dagster/core/telemetry.py
259

this too right?

259

im a bit worried about this - just a lot of pain to be had if we somehow mess up someones instance config accidentally. If we do like this approach we should put it under more extensive testing.

its also a bit odd that we are storing it there but
https://dagster.phacility.com/source/dagster/browse/master/python_modules/dagster/dagster/core/instance/__init__.py has no knowledge of it.

not necessarily in this diff but it might be better to move to a scheme where only user edited

telemetry:
  enabled: True/False

is in the dagster.yaml, and all other metadata is in $DAGSTER_HOME/telemetry next to the log file. Could do something cute like $DAGSTER_HOME/telemtry/id/FILENAME_IS_ID

alangenfeld requested changes to this revision.Thu, Mar 19, 4:53 PM

to your queue for the anonymous case

I do think it might be worth having DagsterInstance in charge the fields stored in dagster.yaml and moving the id out before we ship it - we can discuss that in another medium

This revision now requires changes to proceed.Thu, Mar 19, 4:53 PM

remove programmatic editing for dagster.yaml, move instance_id to $DAGSTER_HOME/telemetry/id.yaml, tested manually. currently updating / adding to test suite

alangenfeld requested changes to this revision.Thu, Mar 19, 10:40 PM

clean up prints

python_modules/dagster/dagster/cli/utils.py
42–43

could even just delete this in the short term

python_modules/dagster/dagster/core/instance/__init__.py
329–334

I would just expose telemetry_enabled instead of the settings

python_modules/dagster/dagster/core/instance/config.py
55

explicitly set default value here

python_modules/dagster/dagster/core/telemetry.py
103–134

as discussed - can clean this up by having

DagsterInstance.get().telemtry_enabled
116–117

we may want to remove this broad try catch in the near opt-in term since we will completely lose the errors that are occurring

This revision now requires changes to proceed.Thu, Mar 19, 10:40 PM
catherinewu marked 4 inline comments as done.

add tests

This revision is now accepted and ready to land.Fri, Mar 20, 1:13 AM
Harbormaster completed remote builds in B8690: Diff 10752.
This revision was automatically updated to reflect the committed changes.