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.

Sign up for GitHub

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

Jump to bottom

Be ok if we can't save a ~/.observablehq file for telemetry #507

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Be ok if we can't save a ~/.observablehq file for telemetry #507

merged 4 commits into from
Jan 11, 2024

Conversation

visnup
Copy link
Member

@visnup visnup commented Jan 11, 2024

@visnup visnup requested review from Fil and mythmon January 11, 2024 20:45
Fil
Fil approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

might be nice to log the error — just for us, when we test and break things?

mythmon
mythmon approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

My initial reaction would be to set the id to some known value if it fails to write (since that implies that user will always be uncorrelated, which is interesting to know). Maybe all zeroes or some known UUID or something. Not a blocker though.

@visnup
Copy link
Member Author

visnup commented Jan 11, 2024 •
edited
Loading

Hm actually maybe nulls (or undefined?) are the right things to return here.

@visnup
Copy link
Member Author

visnup commented Jan 11, 2024 •
edited
Loading

Going to skip the warning log because getPersistentId is a nicely contained helper function and I don't want to deal with passing effects to it so it can log… 😶

Plus, I also feel like telemetry should be as quiet as possible and not get in the way. The only error it currently bothers you with (other than the banner warning) is if you set a different origin and that origin is malformed.

@visnup
Copy link
Member Author

visnup commented Jan 11, 2024

I found an async race condition in the read/write implementation which I fixed. @mythmon @Fil dunno if you want to re-review? I'm thinking I should figure out how to test the implementation of getPersistentId better. An earlier version I had passed in readFile and writeFile in effects instead of the wholesale getPersistentId implementation and maybe that's actually better…

@visnup
Copy link
Member Author

visnup commented Jan 11, 2024

Going to merge this now and consider refactoring getPersistentId more in the future for more test coverage and maybe merge it with loadUserConfig.

@visnup visnup merged commit 3065f75 into main Jan 11, 2024
2 checks passed
@visnup visnup deleted the visnup/telemetry-no-write branch January 11, 2024 22:24
cinxmo pushed a commit that referenced this pull request Jan 12, 2024
@visnup @cinxmo
Be ok if we can't save a ~/.observablehq file for telemetry (#507)
ad9ac49 
* Be ok if we can't save a ~/.observablehq file for telemetry

* Use null IDs if persistence fails

* Fix race condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mythmon mythmon mythmon approved these changes

@Fil Fil Fil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@visnup @Fil @mythmon