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

Copy cortex/pkg/configs package dependency into Loki #5139

Merged
merged 9 commits into from
Jan 27, 2022
Merged

Copy cortex/pkg/configs package dependency into Loki #5139

merged 9 commits into from
Jan 27, 2022

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Jan 13, 2022 •
edited

What this PR does / why we need it:
As a part of removing Loki dependency on cortex packages, we decided to copy the configs package from cortex to Loki itself.
Obs: the huge diff is mostly because of the test suite under configs/legacy_promql.

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

Special notes for your reviewer:
N/A

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@DylanGuedes DylanGuedes changed the title Fork cortex config Copy cortex/pkg/querier package dependency into Loki Jan 13, 2022
@DylanGuedes DylanGuedes changed the title Copy cortex/pkg/querier package dependency into Loki Copy cortex/pkg/configs package dependency into Loki Jan 13, 2022
@DylanGuedes DylanGuedes marked this pull request as ready for review January 14, 2022 11:28
@DylanGuedes DylanGuedes requested a review from a team as a code owner January 14, 2022 11:28
chaudum
chaudum approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

LGTM

cyriltovena
cyriltovena reviewed Jan 24, 2022
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

Can we get rid of the promql_legacy, I'm not a super fan of introducing this one this can be confusing.

I suggest we find out why it's required and get rids of the tests.

@DylanGuedes
Copy link
Contributor Author

DylanGuedes commented Jan 24, 2022 •
edited

Can we get rid of the promql_legacy, I'm not a super fan of introducing this one this can be confusing.

I suggest we find out why it's required and get rids of the tests.

Good point. I looked for it and:

  • Calls to ListAllRuleGroups invokes the method cfg.Config.ParseFormatted, responsible for parsing rules configuration
  • The rules configuration might be in the two versions: v2 (recent one) and v1 (legacy_promql). Current ParseFormatted supports both

So yes, we have the option to drop support for v1 and that would allow us to drop legacy_promql. WDYT?

@kavirajk
Copy link
Collaborator

+1 to remove promql_legacy dependency. It basically boils down to, do we still need to support rules according to the Prometheus 1.x rule format. I think not.

  1. Are we using legacy rule anywhere internally? does it break?
  2. I think it needs an update on changelog and upgrade guide as well?

kavirajk
kavirajk approved these changes Jan 26, 2022
View reviewed changes
Copy link
Collaborator

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM. nice work @DylanGuedes 👍

docs/sources/upgrading/_index.md
@@ -33,6 +33,38 @@ The output is incredibly verbose as it shows the entire internal config struct u

### Loki

#### Dropped support for old Prometheus rules configuration format
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :)

@kavirajk
Copy link
Collaborator

@cyriltovena can you take a look again? (promql_legacy is removed now)

cyriltovena
cyriltovena approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@DylanGuedes
Fork cortex configs into Loki.
74e770c
@DylanGuedes
Use new forked configs package.
34506f6
@DylanGuedes
Remove duplicated import.
1e6dcca
@DylanGuedes
Fix lint issues.
007c068
@DylanGuedes
Use new forked util/logger.
265c356
@DylanGuedes
Drop support for promql rules v1.
c15ab6a 
- This allow us to remove the `legacy_promql` implementation
- If a rulesconfig is received in v1 format, we output a specific error,
  so we can differentiate between a unknown version and a v1 one.
@DylanGuedes
Modify tests to expect error for legacy rules format.
80d52d1
@DylanGuedes
Add changelog and upgrading guide.
a6293a7 
- Added them as we dropped support for legacy promql rules configuration
  which might be a breaking change for some environments.
@DylanGuedes
Update vendor/modules after rebasing.
5992a73
@kavirajk kavirajk merged commit 3d135e5 into grafana:main Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaudum chaudum chaudum approved these changes

@cyriltovena cyriltovena cyriltovena approved these changes

@kavirajk kavirajk kavirajk approved these changes

@KMiller-Grafana KMiller-Grafana Awaiting requested review from KMiller-Grafana

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DylanGuedes @kavirajk @chaudum @cyriltovena