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.

Sign up for GitHub

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

Jump to bottom

*: match naming convention in metrics and documentation #3309

Merged
merged 10 commits into from
Nov 12, 2020
Merged

*: match naming convention in metrics and documentation #3309

merged 10 commits into from
Nov 12, 2020

Conversation

Sayrus
Copy link
Contributor

@Sayrus Sayrus commented Oct 13, 2020 •
edited by kakkoyun
Loading

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.
    ⚠️ This is a breaking change

Changes

Rename metrics not following the components naming convention:

  • thanos_compact_* (from thanos_compactor_*)
  • thanos_rule_* (from thanos_ruler_*)
  • thanos_query_* (from thanos_querier_*)

Also match the naming convention in the documentation and code comments

https://thanos.io/tip/contributing/contributing.md/#components-naming

Verification

Has yet to be tested.

Note

I feel like removing the metrics directly before any deprecation cycle has been done is a bit harsh. But before working on duplicating the two metrics, I'd like to have your opinion on this.

@Sayrus Sayrus force-pushed the thanos-match-compactor-naming branch from a283041 to 1b1cda7 Compare October 13, 2020 08:53
bwplotka
bwplotka reviewed Oct 20, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

I think this is a fair change, but let's commit this right before 0.17.0 and announce it properly. cc @metalmatze next release shepherd. Also worth to change other components to have one breaking release changing metrics not man (:

@Sayrus
Copy link
Contributor Author

Sayrus commented Oct 21, 2020

Makes sense.

So I guess it's time to fix references to thanos_ruler and thanos_querier.
I did not find any thanos_gateway. There might be some in the documentation, I'll check later.

Also, we need to add Thanos Query Frontend to the component naming! (query_frontend and Query Frontend?)

@bwplotka
Copy link
Member

correct! (:

@Sayrus
Copy link
Contributor Author

Sayrus commented Oct 21, 2020

Shall we also update the CHANGELOG.md to retroactively reflect those changes?

@Sayrus Sayrus force-pushed the thanos-match-compactor-naming branch from 0912f3c to 6538553 Compare October 21, 2020 17:59
@Sayrus Sayrus changed the title compact: match components naming in code and metrics *: matchnaming convention in metrics and documentation Oct 21, 2020
@Sayrus Sayrus changed the title *: matchnaming convention in metrics and documentation *: match naming convention in metrics and documentation Oct 21, 2020
kakkoyun
kakkoyun reviewed Oct 23, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this.

You just need to rebase and update the PR description to match recent changes.
Then we can merge this.

And also it'd be nice to add the link to the PR for record keeping purposes https://thanos.io/tip/contributing/contributing.md/#components-naming

@Sayrus Sayrus force-pushed the thanos-match-compactor-naming branch from 6538553 to deaf8ae Compare October 23, 2020 16:51
@Sayrus
Copy link
Contributor Author

Sayrus commented Oct 23, 2020

Hello @kakkoyun ,

This should be good now!

I'm not sure how to link this PR in the CONTRIBUTING.md page. Simply linking it without context or anything would be annoying for users simply reading the page.
Can I let you add it without your own words?

@Sayrus Sayrus mentioned this pull request Oct 29, 2020
Merged
2 tasks
kakkoyun
kakkoyun approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for this.

Let's give a chance to @bwplotka for having a final look and then we can merge it.

bwplotka
bwplotka approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, but metric changes scares me a lot 😱 If someone has alerts or dashboards they don't want to change too often, they will kill us 😢

@Sayrus
Copy link
Contributor Author

Sayrus commented Oct 30, 2020 •
edited
Loading

Thanks, LGTM, but metric changes scares me a lot scream If someone has alerts or dashboards they don't want to change too often, they will kill us cry

That's why I said:

I feel like removing the metrics directly before any deprecation cycle has been done is a bit harsh. But before working on duplicating the two metrics, I'd like to have your opinion on this.

We could duplicate the metrics for one version as a deprecation cycle. However, it will still break the metric history 😞
I don't know what the best / good solution is.

Mathis Raguin and others added 10 commits November 12, 2020 12:34
@Sayrus
pkg/compact: Fix compactor name in comments
f1add14 
Rename compact to compactor

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
pkg/compact: Rename metrics to thanos_compact_*
4e32ee0 
Some metrics used to be under thanos_compactor_*

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
rule: match metrics name with convention
74baa77 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
rule: match documentation with naming convention
9a416bb 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
query: match metrics name with convention
141073c 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
query: match documentation with naming convention
e5f1ea2 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
query: fix naming convention in examples
60a60e7 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
CONTRIBUTING: update naming convention with new query frontend compon…
e4b7924 
…enent

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
update changelog for concerned metrics
499e0c2 
Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus
compact: match naming convention for compactor
6f1fd1f 
tests were moved since the PR was created.

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
@Sayrus Sayrus force-pushed the thanos-match-compactor-naming branch from c1c2078 to 6f1fd1f Compare November 12, 2020 11:38
@Sayrus
Copy link
Contributor Author

Sayrus commented Nov 12, 2020

Rebased on master as one metric name was changed

bwplotka
bwplotka approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

I would say let's go with this. @metalmatze is it fine to get into release?

metalmatze
metalmatze approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Seems good to me. Let's get this into v0.17 then

@metalmatze metalmatze merged commit 6889ae8 into thanos-io:master Nov 12, 2020
Oghenebrume50 pushed a commit to Oghenebrume50/thanos that referenced this pull request Dec 7, 2020
@Sayrus @Oghenebrume50
*: match naming convention in metrics and documentation (thanos-io#3309)
505e6d4 
* pkg/compact: Fix compactor name in comments

Rename compact to compactor

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* pkg/compact: Rename metrics to thanos_compact_*

Some metrics used to be under thanos_compactor_*

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* rule: match metrics name with convention

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* rule: match documentation with naming convention

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* query: match metrics name with convention

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* query: match documentation with naming convention

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* query: fix naming convention in examples

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* CONTRIBUTING: update naming convention with new query frontend componenent

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* update changelog for concerned metrics

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>

* compact: match naming convention for compactor

tests were moved since the PR was created.

Signed-off-by: Mathis Raguin <mathis@cri.epita.fr>
Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@metalmatze metalmatze metalmatze approved these changes

@kakkoyun kakkoyun kakkoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sayrus @bwplotka @kakkoyun @metalmatze