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

Alert section for freshness checks #21676

Merged
merged 8 commits into from
May 16, 2024

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented May 6, 2024

Adds an alerts section to the freshness guide.

I originally wanted to make more sweeping changes here, but it's proving more difficult than I thought. I think the biggest outstanding problem with the guide is thelack of reference to alerting (main important thing about freshness, so let's fix that first).

@graphite-app graphite-app bot added the area: docs Related to documentation in general label May 6, 2024
@graphite-app graphite-app bot requested a review from erinkcochran87 May 6, 2024 23:25
Copy link
Contributor Author

dpeng817 commented May 6, 2024

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

Join @dpeng817 and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented May 6, 2024

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-d1ddv1u4c-elementl.vercel.app
https://dpeng817-alert-section-freshness-docs.dagster.dagster-docs.io

Direct link to changed pages:

@dpeng817 dpeng817 requested a review from sryza May 6, 2024 23:35
@dpeng817 dpeng817 force-pushed the dpeng817/alert_section_freshness_docs branch from 5b487f9 to e316d9e Compare May 7, 2024 22:26
@dpeng817 dpeng817 force-pushed the dpeng817/alert_section_freshness_docs branch from e316d9e to dd26305 Compare May 14, 2024 20:19
Copy link
Contributor

@sryza sryza left a comment

Choose a reason for hiding this comment

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

Should we instead add this section to the asset checks page and link to it from the freshness checks page?

Copy link
Contributor

@erinkcochran87 erinkcochran87 left a comment

Choose a reason for hiding this comment

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

Two small things, but lgtm!

@erinkcochran87
Copy link
Contributor

Should we instead add this section to the asset checks page and link to it from the freshness checks page?

@sryza Which asset check page - this main category one, or this guide for defining checks? What's the thinking behind moving it?

@sryza
Copy link
Contributor

sryza commented May 15, 2024

@sryza Which asset check page - this main category one, or this guide for defining checks?

I honestly didn't think that far - I could see either, or even a third one.

What's the thinking behind moving it?

These instructions aren't really specific to freshness checks in particular. They apply for other asset checks as well.

@dpeng817 dpeng817 force-pushed the dpeng817/alert_section_freshness_docs branch from dd26305 to d6a4db7 Compare May 15, 2024 20:49
@erinkcochran87
Copy link
Contributor

@dpeng817 Back to you - lmk what you think

Copy link
Contributor Author

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

@erinkcochran87 a few small comments otherwise LGTM. Thanks for putting this together.

@erinkcochran87
Copy link
Contributor

Just waiting for BK, then I'll merge this guy

@erinkcochran87 erinkcochran87 merged commit 3354277 into master May 16, 2024
1 of 2 checks passed
@erinkcochran87 erinkcochran87 deleted the dpeng817/alert_section_freshness_docs branch May 16, 2024 16:14
nikomancy pushed a commit that referenced this pull request May 22, 2024
Adds an alerts section to the freshness guide. 

I originally wanted to make more sweeping changes here, but it's proving
more difficult than I thought. I think the biggest outstanding problem
with the guide is thelack of reference to alerting (main important thing
about freshness, so let's fix that first).

---------

Co-authored-by: Erin Cochran <erin.k.cochran@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants