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

[ui] Reject throttleLatest promise with a string instead of an Error #26452

Closed
wants to merge 1 commit into from

Conversation

hellendag
Copy link
Member

@hellendag hellendag commented Dec 12, 2024

Summary & Motivation

When throttleLatest rejects the Promise for an active call, use a string instead of an Error to avoid surfacing unnecessary errors in our error tracking. Since the Promise rejection represents the function behaving as intended, the Error doesn't need to be tracked or raise any alarms.

How I Tested These Changes

Repro the Promise rejection case by viewing a Job dag that has surfaced it. Verify that an error appears in the console. Verify that with this change, no error is surfaced.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hellendag hellendag marked this pull request as ready for review December 12, 2024 19:33
Copy link

github-actions bot commented Dec 12, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-mv0sv1wju-elementl.vercel.app
https://dish-quiet-throttle-reject.core-storybook.dagster-docs.io

Built with commit 511230f.
This pull request is being automatically deployed with vercel-action

@hellendag hellendag force-pushed the dish/quiet-throttle-reject branch from d98f3cd to 511230f Compare December 12, 2024 19:45
@salazarm
Copy link
Contributor

I think the actual issue is that the promise rejection was uncaught.

Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

Can you confirm that the existing code still causes a DD error because I had landed a change to catch the promise error to stop DD from reporting it and curious if that worked or not (it didnt make it into the release).

@hellendag
Copy link
Member Author

Rejection catch should handle this as noted, closing.

@hellendag hellendag closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants