Updated Cortex architecture doc and documented the experimental blocks storage #1945

pracucci · 2020-01-02T14:31:59Z

What this PR does:
In this PR:

Added missing TSDB-related changes (already merged) to changelog
Updated Cortex architecture doc
Documented the experimental blocks storage

Notes to the reviewer:

I suggest to read the updated architecture doc entirely and not from the diff, to have a better overview of how has been covered
Mentioned the WAL, since I do expect to be merged soon.
Mentioned the blocks compactor, which I hope to see merged soon (PR Introduce TSDB blocks compactor #1942).
I've specified the blocks storage config giving preference to YAML and mentioning each related CLI flag. I've used the Prometheus config documentation syntax (also used by Loki). If there are no objection, I would like to submit a subsequent PR to document the entire Cortex config the same way.

Which issue(s) this PR fixes:
N/A

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci · 2020-01-02T14:36:00Z

Specific requests:

@codesome may you double check the WAL section, please?
@owen-d may you double check the query frontend section, please?
@jtlisi may you help me enriching the ruler and alert manager doc, please?
@thorfour may you review the TSDB related stuff?

owen-d · 2020-01-02T15:15:57Z

Nice work with the docs. Frontend section is looking good.

docs/architecture.md

gotjosh

As there's plenty of changes in this document, I want to avoid giving an all-at-once review.

Is this is the kind of feedback you expected? Let me know and I'll keep going.

Overall, fantastic job @pracucci 👌

CHANGELOG.md

docs/architecture.md

thorfour

minor nit around some wording otherwise LGTM from the TSDB side of things.

docs/architecture.md

pracucci · 2020-01-02T16:32:30Z

Thanks @owen-d and @gotjosh. I've addressed your comments. May you re-check it, please?

Is this is the kind of feedback you expected? Let me know and I'll keep going.

@gotjosh Yes, definitely. I'm just a bit dubious about the "Blocks" and "Chunks" uppercase letter. If we decide to go into this direction, it's better if we decide the rationale and I take care of all the changes cause there are many (more than then the places you edited). So far, I've kept them lowercase.

gotjosh

Sorry Marco, for leaving you with more comments 🙈. Safe to say my comments are optional - feel free to pick the ones that make sense to you.

Overall, great job on these docs - the improvement is superb! 🍾

docs/architecture.md

gotjosh · 2020-01-02T18:04:55Z

docs/architecture.md


 #### The hash ring

-A consistent hash ring is stored in [Consul](https://www.consul.io/) as a single key-value pair, with the ring data structure also encoded as a [Protobuf](https://developers.google.com/protocol-buffers/) message. The consistent hash ring consists of a list of tokens and ingesters. Hashed values are looked up in the ring; the replication set is built for the closest unique ingesters by token. One of the benefits of this system is that adding and remove ingesters results in only 1/_N_ of the series being moved (where _N_ is the number of ingesters).


In my opinion, this section is one slightly more complex pieces of Cortex. You've done a fantastic job trying to expand on it, but I'm not sure about the way it reads.

If you feel strongly about it we can leave it as is, if you have doubts - I'm happy to take a stab and try to reword it and perhaps add a small diagram to it. What do you think?

I never feel very strong about something, and I'm always open to discussion 😉 That being said, I would appreciate if could express the same concept in a easier way.

About the diagram, sure. I also thought about it, but I didn't (yet) because I'm not sure how to draw a diagram in a way which will be easily editable by other contributors in the future (ie. which tool to use? where should we share the sources?).

I tend to use mermaid for that exact same reasons. The source code is extremely simple which makes it easy to check it into version control and they have a live editor which means anyone can copy/paste then start editing right away.

Let's merge this as is and I'll create a follow-up issue assigned to me to come up with something.

I have created #1949 to record this conversation. Let me know what you think.

I think it's good not blocking this PR with the improved ring description. I will comment the issue.

gotjosh · 2020-01-02T18:11:17Z

@gotjosh Yes, definitely. I'm just a bit dubious about the "Blocks" and "Chunks" uppercase letter. If we decide to go into this direction, it's better if we decide the rationale and I take care of all the changes cause there are many (more than then the places you edited). So far, I've kept them lowercase.

@pracucci I'll leave that in your hands then. My opinion is that if these components have their own semantics they should be consider proper nouns e.g. A Block within Cortex does not fall under the regular definition of the word.

As I linked in the Prometheus doc, not capitalizing makes sense if we refer to them as adjectives e.g. block files or index files - in every other case, they should.

pracucci · 2020-01-03T08:27:30Z

As I liked in the Prometheus doc, not capitalizing makes sense if we refer to them as adjectives e.g. block files or index files - in every other case, they should.

Ok @gotjosh. May you check it again, please? I've capitalized them - when they're nouns (at least I believe they are) - in the commit 0e481f3

gotjosh

I reviewed the architecture doc from Storage to right before Quorum consistency. Looks good from my side 👌

CHANGELOG.md

docs/architecture.md

gouthamve · 2020-01-03T14:02:11Z

docs/architecture.md


 ## Services

-Cortex has a service-based architecture, in which the overall system is split up into a variety of components that perform specific tasks and run separately (and potentially in parallel).
+Cortex has a service-based architecture, in which the overall system is split up into a variety of components that perform a specific task, run separately, and in parallel.


Maybe mention that you can run all of them together in a single process mode too?

Right. What's about now?

docs/architecture.md

gouthamve · 2020-01-03T14:15:36Z

docs/architecture.md


-> This hashing scheme was originally chosen to reduce the number of required ingesters on the query path. The trade-off, however, is that the write load on the ingesters is less even.
+The first hashing strategy (default) was originally chosen to reduce the number of required ingesters on the query path, but running Cortex in production showed that an evenly distributed write load on the ingesters is preferable at the cost of always querying most of the ingesters.


I would say that this is mostly us tbh. We might also be the only ones who are running it this way. Others are still using the defaults and it works for them. For us, it was sparked by one big customer that had an huge number of metrics with the same label-name.

I think the doc makes it sound like the shard-by-all should be the way to go for everyone, but for most people the defaults should be fine.

I simplified it, removing "opinions" and just leaving the trade-off mention.

gouthamve · 2020-01-03T14:33:21Z

docs/architecture.md


-### Query frontend
+Contrary to the sole replication and given the persistent local disk data is not lost, in the event of multiple ingesters failure each ingester will recover the in-memory series samples from WAL upon subsequent restart. The replication is still recommended in order to ensure no temporary failures on the read path in the event of a single ingester failure.


Contrary to the sole replication --> what does this mean?

It means, "differently then when you setup a cluster only with the replication and without the WAL"

docs/architecture.md

Signed-off-by: Marco Pracucci <[email protected]>

pracucci · 2020-01-07T11:48:03Z

@gotjosh @gouthamve I've addressed your comments and replied to a couple. May you take another look, please?

gotjosh

Looks good from here @pracucci! 🥇

bboreham

Great, thanks!

owen-d reviewed Jan 2, 2020

View reviewed changes

docs/architecture.md Outdated Show resolved Hide resolved

owen-d reviewed Jan 2, 2020

View reviewed changes

docs/architecture.md Outdated Show resolved Hide resolved

gotjosh reviewed Jan 2, 2020

View reviewed changes

thorfour approved these changes Jan 2, 2020

View reviewed changes

docs/architecture.md Outdated Show resolved Hide resolved

pracucci force-pushed the doc-experimental-tsdb-so-far branch 4 times, most recently from 689c800 to f86df9c Compare January 2, 2020 16:30

gotjosh approved these changes Jan 2, 2020

View reviewed changes

pracucci force-pushed the doc-experimental-tsdb-so-far branch from f399e7e to ac912b6 Compare January 2, 2020 18:43

gotjosh approved these changes Jan 3, 2020

View reviewed changes

gotjosh mentioned this pull request Jan 3, 2020

Docs: Update the Architecture Section about the Ring #1949

Closed

gouthamve reviewed Jan 3, 2020

View reviewed changes

pracucci added 5 commits January 7, 2020 12:32

Added missing TSDB-related changes to changelog

f009951

Signed-off-by: Marco Pracucci <[email protected]>

Updated Cortex architecture doc

01f128f

Signed-off-by: Marco Pracucci <[email protected]>

Documented the experimental blocks storage

13da9d2

Signed-off-by: Marco Pracucci <[email protected]>

Updated architecture doc based feedback

d213a61

Signed-off-by: Marco Pracucci <[email protected]>

Capitalized Block and Chunk words when nouns

72e47bf

Signed-off-by: Marco Pracucci <[email protected]>

pracucci force-pushed the doc-experimental-tsdb-so-far branch from 0e481f3 to 72e47bf Compare January 7, 2020 11:32

Updated architecture doc

71e08e5

Signed-off-by: Marco Pracucci <[email protected]>

gotjosh approved these changes Jan 7, 2020

View reviewed changes

bboreham approved these changes Jan 7, 2020

View reviewed changes

bboreham merged commit 1a3b008 into cortexproject:master Jan 7, 2020

pracucci deleted the doc-experimental-tsdb-so-far branch January 7, 2020 14:04

pracucci mentioned this pull request Feb 4, 2020

Update/enhance architecture documentation #1375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Cortex architecture doc and documented the experimental blocks storage #1945

Updated Cortex architecture doc and documented the experimental blocks storage #1945

pracucci commented Jan 2, 2020 •

edited

Loading

pracucci commented Jan 2, 2020

owen-d commented Jan 2, 2020

gotjosh left a comment •

edited

Loading

thorfour left a comment

pracucci commented Jan 2, 2020

gotjosh left a comment

gotjosh Jan 2, 2020

pracucci Jan 2, 2020

gotjosh Jan 3, 2020

gotjosh Jan 3, 2020

pracucci Jan 7, 2020

gotjosh commented Jan 2, 2020 •

edited

Loading

pracucci commented Jan 3, 2020

gotjosh left a comment

gouthamve Jan 3, 2020

pracucci Jan 7, 2020

gouthamve Jan 3, 2020

pracucci Jan 7, 2020

gouthamve Jan 3, 2020

pracucci Jan 7, 2020

pracucci commented Jan 7, 2020 •

edited

Loading

gotjosh left a comment

bboreham left a comment


		#### The hash ring

		A consistent hash ring is stored in [Consul](https://www.consul.io/) as a single key-value pair, with the ring data structure also encoded as a [Protobuf](https://developers.google.com/protocol-buffers/) message. The consistent hash ring consists of a list of tokens and ingesters. Hashed values are looked up in the ring; the replication set is built for the closest unique ingesters by token. One of the benefits of this system is that adding and remove ingesters results in only 1/_N_ of the series being moved (where _N_ is the number of ingesters).


		> This hashing scheme was originally chosen to reduce the number of required ingesters on the query path. The trade-off, however, is that the write load on the ingesters is less even.
		The first hashing strategy (default) was originally chosen to reduce the number of required ingesters on the query path, but running Cortex in production showed that an evenly distributed write load on the ingesters is preferable at the cost of always querying most of the ingesters.


		### Query frontend
		Contrary to the sole replication and given the persistent local disk data is not lost, in the event of multiple ingesters failure each ingester will recover the in-memory series samples from WAL upon subsequent restart. The replication is still recommended in order to ensure no temporary failures on the read path in the event of a single ingester failure.

Updated Cortex architecture doc and documented the experimental blocks storage #1945

Updated Cortex architecture doc and documented the experimental blocks storage #1945

Conversation

pracucci commented Jan 2, 2020 • edited Loading

pracucci commented Jan 2, 2020

owen-d commented Jan 2, 2020

gotjosh left a comment • edited Loading

Choose a reason for hiding this comment

thorfour left a comment

Choose a reason for hiding this comment

pracucci commented Jan 2, 2020

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh commented Jan 2, 2020 • edited Loading

pracucci commented Jan 3, 2020

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Jan 7, 2020 • edited Loading

gotjosh left a comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

pracucci commented Jan 2, 2020 •

edited

Loading

gotjosh left a comment •

edited

Loading

gotjosh commented Jan 2, 2020 •

edited

Loading

pracucci commented Jan 7, 2020 •

edited

Loading