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
Add Cortex and Prometheus Remote Write exporter design #1464
Add Cortex and Prometheus Remote Write exporter design #1464
Changes from 1 commit
ac39113
2abf604
407c536
7b2ad3f
039bbbd
dd390ea
7f8529a
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.
Can we rename this from Cortex exporter to "Prometheus Remote Write exporter"? Cortex just uses the Prom RW APIs and by renaming we'll be clearer in intent that we support Prom RW and not just Cortex.
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, we do want to emphasize the support for Cortex, but we definitely can indicate its compatibility with other remote write backends in our document. Thanks for the suggestion. Also, a question on remote write: does it send out sorted label set and how do we treat duplicate labels?
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 send a list of labels. And I think its upto the upstream implementation to deal with the discrepancy. In general, in Prometheus, you can never have a label name that has 2 values, so the upstream will likely reject it with a 4xx. I am fairly certain that the labels need not be sorted.
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.
Thanks for the reply. Another related question: for Cortex to work, should our exporter meet the requirement that metric of the same type and name must have the same labels? If so, how could we maintain it in the case of multiple exporters?
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.
OTel gives some specification about duplicate labels. They should be eliminated before the exporter sees them. I'm not sure if the SDK specification will say they're sorted, but in the OTel Go SDK they are sorted as an efficient means of deduplication.
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.
Would it be easier to vendor in Prometheus remote write package here?
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.
Thanks for the suggestion, using the client from remote write makes sense. We are also trying to allow users to specify any header or pass in a http.Client to send requests. I will take a look at whether this is possible with remote write package.
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.
It doesn't sadly, but we should see if we can make the current client accept a custom
http.Client
.https://github.com/prometheus/prometheus/blob/master/storage/remote/client.go#L125-L133
We can try adding a
SetHTTPClient()
method to the Prometheus client object.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, I think that change would be very helpful for our usage.