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

Move components names to Ruler, Compactor, Querier (-er) #1871

Closed
bwplotka opened this issue Dec 11, 2019 · 23 comments
Closed

Move components names to Ruler, Compactor, Querier (-er) #1871

bwplotka opened this issue Dec 11, 2019 · 23 comments

Comments

@bwplotka
Copy link
Member

bwplotka commented Dec 11, 2019

It's maybe a minor but we should be consistent on naming. In the documentation, code and thanos command we have rule, compact, downample, query vs in some docs, deployments and most of the presentations we are leaking Querier, Ruler, Compactor

This conflicts a bit with Store Gateway but we already have some discussion about renaming it: #1646

I would vote -er as it's more correct (aka go way of naming objects as well) and it avoids name collisions like rule (recording/alerting rule) vs rule (the component responsible for evaluating rules).

AC:
< to agree>

Any thoughts?

@GiedriusS
Copy link
Member

GiedriusS commented Dec 11, 2019

It makes sense, I agree. Also, don't forget our GH labels! 😄

@domgreen
Copy link
Contributor

Why not also change store to gateway or proxyer whilst making changes?

@bwplotka
Copy link
Member Author

Same for receive -> receiver I guess (:

@yeya24
Copy link
Contributor

yeya24 commented Dec 13, 2019

Anyone work on this issue? I guess I can take this 😆

@yeya24 yeya24 mentioned this issue Dec 13, 2019
2 tasks
@bwplotka
Copy link
Member Author

Thanks. We need to decide what to do with metrics as well as they are all over the place with query rule etc, should we rename at once? @brancz any tips? (:

@bwplotka
Copy link
Member Author

I think we need to be careful here as this minor consistency naming change might be not worth everywhere when introducing so many breaking changes ): Things like metrics, components will make our user life harder when upgrading.

@yeya24
Copy link
Contributor

yeya24 commented Dec 16, 2019

I think updating the metrics now might not be a good option. Just update the docs first.

@bwplotka
Copy link
Member Author

👍

@brancz
Copy link
Member

brancz commented Dec 16, 2019

I feel like for how disruptive renaming the components would be, the gain would be too small. Yes we should be consistent in the metrics and component names, but in docs and presentations for the flow of the language we can use querier, ruler, etc. but otherwise the existing component names work just fine in my opinion and don't require changing.

I don't feel super strongly about this so if the rest of the maintainers feel like this is necessary I won't stand in the way, but I feel it's going to cause more confusion and unexpected breakage than gain.

@daixiang0
Copy link
Member

@kakkoyun any process about this?

@bwplotka
Copy link
Member Author

bwplotka commented Jan 6, 2020

Due to compatibility issues I would say we stick to docs, new places we name things like Alert names, deployment names etc.

Metrics, command, and component names in code should stay for now.

@kakkoyun
Copy link
Member

kakkoyun commented Jan 6, 2020

@daixiang0 This week I'll start working on this and primarily focus on docs.

@yeya24
Copy link
Contributor

yeya24 commented Jan 14, 2020

I am not sure is this still valid?

@kakkoyun
Copy link
Member

@yeya24 Maybe we should break this into pieces. Like thanos-mixin/compact -> thanos-mixin/compactor, docs/compact -> docs/compactor and cmd/compact -> cmd/compactor etc.

WDYT?

@brancz
Copy link
Member

brancz commented Jan 31, 2020

I think we need to revisit our changes slightly here. I was under the assumption we’re only changing language in sentence form but keep technical references consistent so I think the mixin should use the command names in its structure and in let’s say alert descriptions use whatever is more fluent.

@kakkoyun
Copy link
Member

kakkoyun commented Feb 3, 2020

My understanding from this issue was, we would eventually move to *tor naming schema, thus my changes.

And it's more clear now.

Just to recap. we will keep structural build blocks (code references, filenames etc.) as in commands and only rename documentation (user guideline, alert message etc). Am I right?

@bwplotka
Copy link
Member Author

bwplotka commented Feb 3, 2020

Definitely, we have to find something consistent (:

only changing language in sentence form
keep technical references

What exactly this means? I assume:

  • User -er -or in:

    • While speaking about components
    • Presentations
    • Documentation
    • Code comments (?)
    • Log messages, traces?
  • Keep the old way (and revert things that were made -er -or:

    • Metrics
    • Commands
    • Mixin/alerts/rules
    • Container names
    • Flags, configuration?
    • Code methods/structs
    • Package names

There are some gray areas like logging e.g which are not really tricky to change. I think we need to just put some list and agree on this. Anything else I missed?

@brancz
Copy link
Member

brancz commented Feb 3, 2020

That sounds perfect to me. I'll start applying that. Maybe it's worth documenting this in our CONTRIBUTING.md ?

@bwplotka
Copy link
Member Author

bwplotka commented Feb 3, 2020

Definitely will add - and let's make sure we keep it up to date

@bwplotka
Copy link
Member Author

bwplotka commented Feb 3, 2020

Done: #2094

To close this issue we probably have to look through those items and ensure consistency. I know for sure there are metrics that are sometimes compact sometimes compactor etc.

@stale
Copy link

stale bot commented Mar 4, 2020

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

@stale stale bot added the stale label Mar 4, 2020
@brancz
Copy link
Member

brancz commented Mar 4, 2020

I think we’re pretty close to having all the changes down for this. Only a few metric names have to be fixed I think.

@stale stale bot removed the stale label Mar 4, 2020
@stale
Copy link

stale bot commented Apr 3, 2020

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

@stale stale bot added the stale label Apr 3, 2020
@stale stale bot closed this as completed Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants