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

add a diagram to help visualize timer metrics #30650

Merged
merged 4 commits into from
Jun 4, 2023

Conversation

vandonr-amz
Copy link
Contributor

Wrote the diagram in Mermaid to make it easier to maintain.
I'd have written in the metrics rst directly, but mermaid graphs are not supported as of now I think.
I added a link to github to make it somewhat visible from the doc.

There seem to be a plugin to have sphynx generate mermaid graphs, maybe if we set this up we can merge the rst and the md.

@vandonr-amz
Copy link
Contributor Author

Just realized mermaid puts a red bar on the current time in the graph :p
We can fix this either by moving the example outside of typical working hours so that people would not usually see it (0-8am for instance), or maybe by setting it to a date in the past but hiding the day/month from the legend.
Or we do nothing and say that it's good enough like this.

Copy link
Contributor

@eladkal eladkal Apr 14, 2023

Choose a reason for hiding this comment

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

I'm not a fan of user facing documntation being served from the repo. We do it with policies but that is about it.
Serving directly from the repo means links will be broken if we change them also there is no corelation to version releases. Users will always see latest version of the file which my not be relevant for thier Airflow version.

That said, if we are OK with that approch we should at least scope it to a folder where we know that changes to files in that folder might require changes to site documntation.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, as I mentioned in the description, I'd prefer being able to have my mermaid interpreted directly by sphynx. Maybe I can look at setting up the required plugin in the near future...

Copy link
Member

@potiuk potiuk Apr 15, 2023

Choose a reason for hiding this comment

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

Or maybe - we can find a better tool for the user-facing documentation ? The big value of mermaid is that it can be "close" to the code and github will render it during PRs and when you look at the code.
I think @eladkal mentioned some time ago another diagramming tool we can use (can't remember) and if there is a sphinx plugin for example that we can use to generate such docs, and if it has a good free IDE support to render it locally (and allows for more types of user-type diagrams), then maybe we could use another tool?

I tihnk mermaid is great for "internal development tool", but there are better alternatives for "user-facing" diagrams.

Just a quick search revealed this for example:

https://pypi.org/project/sphinx-diagrams/ where you can write Python code to generate architecture diagrams (which is even cooler if we can make diagrams as a Python code, which is precisely what Airflow does with workflows).

Maybe we can find a tool that will fit better here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to check https://github.com/mingrammer/diagrams which is diagram as code and it produce output as image. When changing the code the CI will generate the image accordingly. I wanted to use it for our archetacture diagrams that we have in the docs.

Copy link
Member

@potiuk potiuk Apr 15, 2023

Choose a reason for hiding this comment

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

Yep. That's it - the sphinx-diagram is (incomplete they claim) but sphinx integration for the mingrammer diagrams. There is an issue that tracks it (see this comment: mingrammer/diagrams#2 (comment)) and others wrote a more complete integration with sphinx.

@potiuk
Copy link
Member

potiuk commented Apr 15, 2023

Also see #30656 (comment) - where @hussein-awala explains to a user what happens when the tasks are cleared. I think it would be great if such a documetnation could explain also some more complex cases like those - maybe not immediately, we should start small. but having a good tool selected that will allow us to easily explain such things in a visual way will allow to incrementally add more and more cases explained in similar way.

@vandonr-amz vandonr-amz marked this pull request as draft May 1, 2023 17:37
@vandonr-amz
Copy link
Contributor Author

converting to draft until I have time to take a deeper look at how to integrate this in the doc

@vandonr-amz vandonr-amz marked this pull request as ready for review June 1, 2023 23:14
@vandonr-amz
Copy link
Contributor Author

removed the part changing the public doc. I think this still has value in the github repo, because finding a good solution to generate graphs in the public doc would keep this PR open forever.

@potiuk
Copy link
Member

potiuk commented Jun 4, 2023

Agree. All for it.

@potiuk potiuk merged commit 340f706 into apache:main Jun 4, 2023
@vandonr-amz vandonr-amz deleted the vandonr/nice branch June 5, 2023 16:43
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Jul 6, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants