Page MenuHomePhabricator

Change telemetry to be opt-in instead

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

Diff Detail

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

Event Timeline

catherinewu created this revision.Mar 18 2020, 10:39 PM
alangenfeld added inline comments.Mar 19 2020, 12:03 AM

this too right?


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 has no knowledge of it.

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

  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.Mar 19 2020, 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.Mar 19 2020, 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.Mar 19 2020, 10:40 PM

clean up prints


could even just delete this in the short term


I would just expose telemetry_enabled instead of the settings


explicitly set default value here


as discussed - can clean this up by having


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.Mar 19 2020, 10:40 PM
catherinewu marked 4 inline comments as done.

add tests

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