-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Sidecar: Added matchers support #3940
Conversation
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to fix the failing test cases.
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
cmd/thanos/sidecar.go
Outdated
"err", err, | ||
) | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still thinking about this logic.
This doesn't work well when Prometheus version is < 2.14.0, where /api/v1/status/buildinfo
doesn't exist. In this case, we just get 404s but we are still retrying indefinitely. This seems wrong to me.
We can only retry for a short time or we check the return error to stop retrying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets retry for lets say N
times (maybe N=5
) and exit if retry fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for Prometheus <2.14.0
this function would fail, so we also need to decide how to fetch build version for <2.14.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in contributor hours:
Let’s check for 404, if it’s present, set 0 value for promVersion
Signed-off-by: Namanl2001 <[email protected]>
cmd/thanos/sidecar.go
Outdated
if err != nil { | ||
return errors.Wrap(err, "failed to get prometheus version") | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you run this in a goroutine and it ends, the sidecar program also stops running because of the run group. Please make sure your code doesn't break the functionality. You can see the tests failed.
I think it is unnecessary to have a separate goroutine for checking the Prometheus version as it is unlikely to change.
We can run it in the main thread once? WDYT? @yashrsharma44 @Namanl2001 @bwplotka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a doubt that we already have more than one actor in this run group but why there is no such problem with them?
I think it is unnecessary to have a separate goroutine for checking the Prometheus version as it is unlikely to change.
Do we add things in a separate goroutine which are likely to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments during Community call
cmd/thanos/sidecar.go
Outdated
if err != nil { | ||
return errors.Wrap(err, "failed to get prometheus version") | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version := p.promVersion() | ||
|
||
if len(r.Matchers) == 0 || version > "2.24" { | ||
vals, err = p.client.LabelValuesInGRPC(ctx, p.base, r.Label, r.Matchers, r.Start, r.End) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we making gRPC call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming makes me confused, too. We are making HTTP calls but return GRPC error.
Signed-off-by: Namanl2001 <[email protected]>
As suggested, removed the separate goroutine for fetching the Prometheus version and moved the code block to an existing goroutine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @Namanl2001 Thanks for your patience and great work!
Need another look for this pr to merge. Maybe @yashrsharma44 @bwplotka ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some nits regarding some edge cases when the version
is not a valid string. If this case sounds unlikely, we can go ahead with the current implementation, looks good to me.
pkg/store/prometheus.go
Outdated
) | ||
|
||
vals := []string{} | ||
version := p.promVersion() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.promVersion
might return some garbage value if the m.BuildVersion
doesn't run correctly, so let's make sure version
is a correct version string, and not some garbage value.
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
PHAL! @yeya24 @yashrsharma44 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested some coding style, otherwise LGTM 🔥
pkg/store/prometheus.go
Outdated
version, err := semver.Parse(v) | ||
if err == nil { | ||
baseVer, err := semver.Make("2.24.0") | ||
if err != nil { | ||
return nil, err | ||
} | ||
if version.Compare(baseVer) == 1 { | ||
lvc = true | ||
} | ||
} | ||
|
||
if len(r.Matchers) == 0 || lvc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version, err := semver.Parse(v) | |
if err == nil { | |
baseVer, err := semver.Make("2.24.0") | |
if err != nil { | |
return nil, err | |
} | |
if version.Compare(baseVer) == 1 { | |
lvc = true | |
} | |
} | |
if len(r.Matchers) == 0 || lvc { | |
// Make sure the prometheus version is a valid semver type. | |
version, err := semver.Parse(v) | |
if err != nil { | |
return nil, err | |
} | |
baseVer, err := semver.Make("2.24.0") | |
if err != nil { | |
return nil, err | |
} | |
if len(r.Matchers) == 0 || version.Compare(baseVer) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets simplify things, return early if an error - https://thanos.io/tip/contributing/coding-style-guide.md/#control-structure-prefer-early-returns-and-avoid-else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the version is malformed we can still make a series call instead of returning the error. Please see If it is malformed, we just use the series way
in #3940 (comment)
pkg/store/prometheus.go
Outdated
version, err := semver.Parse(v) | ||
if err == nil { | ||
baseVer, err := semver.Make("2.24.0") | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is true(https://github.com/thanos-io/thanos/pull/3940/files#r622405182), then we should not exit early. Then we can remove the error check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can remove the error check.
Sorry, are you saying about the error check at Line 515? As it is for handling error retuned while initializing the baseVer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, if it errors out, we can revert to our original behaviour of -
we can still make a series call instead of returning the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this look good?
version, err1 := semver.Parse(v)
baseVer, err2 := semver.Make("2.24.0")
if err1 == nil && err2 == nil && version.Compare(baseVer) == 1 {
lvc = true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for elongating this, but we can simplify things by using this package
I checked their docs and it stated for their Compare function, we can provide two strings, if the promVersion is invalid, the value would be 0
, so we dont have to worry about that. So lets use this -
defaultVersion := "2.24.0"
if len(r.Matchers) == 0 || semver.Compare(version, defaultVersion) == 1 {
Our current implementation is making it unnecessarily verbose and by using the above package, we can simplify things 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on this @yeya24? 🤗 As you earlier suggested to use a library #3940 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already use the semver library then let's use it here as well.
Signed-off-by: Namanl2001 <[email protected]>
pkg/store/prometheus.go
Outdated
v := p.promVersion() | ||
|
||
version, err1 := semver.Parse(v) | ||
baseVer, err2 := semver.Make("2.24.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make the base version a global variable and we know it doesn't throw an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we make it a global variable? 🤔 We can still keep baseVer
a local variable and don't check for an error while declaring and initializing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For performance. If you call LabelValues
1k times, then semver.Make("2.24.0")
will be called 1k times. If you make it a global variable then it is parsed only once.
Signed-off-by: Namanl2001 <[email protected]>
Signed-off-by: Namanl2001 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc @yashrsharma44 @bwplotka for a final review. If there is no objection, I can merge it tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💪
Fixes task 2:
For Prometheus store (sidecar), add matchers to the LabelValues query
of #3779Changes
Implemented API
/api/v1/status/buildinfo
which returns the buildinfo of prometheus. Calling this API once when the sidecar starts and using it inpkg/store/prometheus.go
labelValues
method, if the version is> "2.24"
used labelValues query otherwise fallback to series query.Verification
Added unit tests