Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sidecar: Added matchers support #3940
Changes from 9 commits
00e9eb2
51a162d
f4096da
f4069b7
6c2ec33
a4073e4
fe5067a
92c3234
df6b6ef
aaf7906
ef797f6
fd708fe
d874bc1
e59208c
893e37c
92570d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 (maybeN=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
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?
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.
We could put it somewhere here: https://github.com/thanos-io/thanos/pull/3940/files#diff-a89173b10deb4d469ed5524c1addf5b154030c04f1001f9dc91676b7966f1127R157
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 them.BuildVersion
doesn't run correctly, so let's make sureversion
is a correct version string, and not some garbage value.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.