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

vendor: Add dependency on prometheus #70325

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

rimadeodhar
Copy link
Collaborator

This PR adds an external dependency on prometheus. We need
the promql library in order to enforce validity of promql
expressions which will be contained in upcoming alerting
and aggregation rules. These rule implementations are
upcoming as a part of the new metrics upgrade.

Resolves #69796

Release note: None

@rimadeodhar rimadeodhar requested review from a team September 16, 2021 17:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@rimadeodhar rimadeodhar left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)


go.mod, line 141 at r1 (raw file):

	github.com/prometheus/common v0.10.0
	github.com/prometheus/procfs v0.6.0 // indirect
	github.com/prometheus/prometheus v2.5.0+incompatible

We have to use this old dependency because of our dependency on the an old zipkin version https://github.com/cockroachdb/cockroach/blob/master/go.mod#L131. Using later versions of prometheus upgrades the zipkin version which breaks our build. I think our zipkin version should be upgraded but that is out of scope for this PR.

@knz
Copy link
Contributor

knz commented Sep 18, 2021

NB: it's rather unusual to create a PR to import a dependency before it's actually used in the code.

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

NB: it's rather unusual to create a PR to import a dependency before it's actually used in the code.

@knz do you know the reason for this? In the case of this PR we have an RFC which clearly outlines the need. I like having a separate dep PR so you can test vendor changes in isolation and make the code review for the actual work easier to look at.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rimadeodhar)


go.mod, line 141 at r1 (raw file):

Previously, rimadeodhar (Rima Deodhar) wrote…

We have to use this old dependency because of our dependency on the an old zipkin version https://github.com/cockroachdb/cockroach/blob/master/go.mod#L131. Using later versions of prometheus upgrades the zipkin version which breaks our build. I think our zipkin version should be upgraded but that is out of scope for this PR.

Gotcha. Thanks for clarifying. We should consider upgrading Zipkin at some point as well, if you run into any issues w/ this old prom dependency.

@rimadeodhar
Copy link
Collaborator Author

TFTR!
bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Sep 20, 2021

Build succeeded:

@craig craig bot merged commit bfdcfa0 into cockroachdb:master Sep 20, 2021
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.

metrics: Import prometheus PromQL parser as external library.
4 participants