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

bug-1928847: Update architecture diagram for GCP #6799

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

relud
Copy link
Member

@relud relud commented Nov 7, 2024

stage submitter now reads from gcs and pubsub, and writes to the collector's load balancer. webapp and crontabber now show that they read processed crashes from gcs. I split up gcs into raw and processed crashes to limit the number of crossing lines, but kept a single pubsub to also limit crossing lines.

With more crossing lines I couldn't visually track orthogonal lines anymore, so I switched to straight lines and reorganized things as needed, with a secondary goal of data flowing left to right and top to bottom where possible (e.g. data still flows up for telemetry to reduce line crossing).

based on the legend stage submitter writes to crash-reports and processor "writes" to the version string api, because if I switch the arrow to have processor read the api it looks like the load balancer is routing version string api requests to the processor.

@relud relud requested a review from willkg November 7, 2024 22:40
@relud relud requested a review from a team as a code owner November 7, 2024 22:40
@willkg
Copy link
Contributor

willkg commented Nov 8, 2024

I have a couple of thoughts:

  1. I find the new diagram really hard to read and reason about.
  2. It feels like this isn't an architecture diagram. The arrows switched from being about connections (Thing A connects to Thing B) and turned into data flow.

We should have a data flow diagram for crash data--covered in bug 1769978--but that should be separate from the architecture diagram.

What does everyone else think? Does the new diagram help you understand the Socorro architecture?

@relud
Copy link
Member Author

relud commented Nov 8, 2024

I find the new diagram really hard to read and reason about.

I don't know how to materially turn this feedback into diagram changes. I agree, but I also thought that about the old diagram. I could hand this off so someone else can give it a shot instead?

I feel like part of the problem is that almost every component accesses both gcs raw/processed and pubsub, and showing those connections creates a lot of overlapping lines. maybe i could separate those and put them in a box that with a note that says "used by every component"?

It feels like this isn't an architecture diagram. The arrows switched from being about connections (Thing A connects to Thing B) and turned into data flow.

I was under the impression the arrows already indicated data flow. Were the arrows are supposed to be like source--uses-->destination? in that case i would remove the legend, and have everything pointing away from kuberenetes components except the gclbs?

@relud relud force-pushed the relud-bug-1928847-update-arch-diag branch from 9307258 to 2c5a588 Compare November 8, 2024 18:22
@relud
Copy link
Member Author

relud commented Nov 8, 2024

I updated the diagram to apply my own suggestions from my last comment, plus switch back to orthogonal lines, lmk if that helps?

@relud relud force-pushed the relud-bug-1928847-update-arch-diag branch 3 times, most recently from ea7563f to 998a1d3 Compare November 8, 2024 18:44
@relud relud force-pushed the relud-bug-1928847-update-arch-diag branch from 998a1d3 to 10c97a2 Compare November 8, 2024 18:47
@willkg
Copy link
Contributor

willkg commented Nov 18, 2024

@biancadanforth , @smarnach : ^^^ what do you two think?

@smarnach
Copy link
Contributor

I only looked at the latest version of the diagram, and I like it. There's only so much you can encode in a single diagram, and I think this diagram strikes a nice balance. The only thing I found slightly confusing is the arrow pointing back to the load balancer from the stage submitter, since it looks kind of circular, while in reality that arrow goes from the stage submitter running in prod to the stage load balancer. I don't know how to improve this, though – maybe we could just add some comment to that arrow?

More generally, there are different kinds of architecture diagrams for software. The goal here appears to be visualizing the infrastructure of Socorro, which I think is useful. This kind of diagram usually also visualizes the data flow as well, since there just isn't too big a difference between "component A accesses component B" and "data flows from component A to component B". Either way, I think the current version of the diagram is useful.

@relud
Copy link
Member Author

relud commented Nov 20, 2024

The only thing I found slightly confusing is the arrow pointing back to the load balancer from the stage submitter, since it looks kind of circular, while in reality that arrow goes from the stage submitter running in prod to the stage load balancer. I don't know how to improve this, though – maybe we could just add some comment to that arrow?

It actually lives in stage but reads from prod, so I think i'll change it to have that connecting from "off-screen" indicating it's reading prod crashes and pubsub standard queue, and also add (stage only) under the STAGE SUBMITTER text.

@relud

This comment was marked as resolved.

@smarnach
Copy link
Contributor

Thanks! I like the new version with the arrow pointing off-screen better.

@willkg
Copy link
Contributor

willkg commented Nov 21, 2024

Having the arrow point "off-screen" sounds good to me as well. I like this diagram. 💯

Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@relud relud added this pull request to the merge queue Nov 22, 2024
Merged via the queue into main with commit 442c45e Nov 22, 2024
1 check passed
@relud relud deleted the relud-bug-1928847-update-arch-diag branch November 22, 2024 17:15
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.

3 participants