-
Notifications
You must be signed in to change notification settings - Fork 79
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
Exporters: add prometheus remote write exporter #177
base: master
Are you sure you want to change the base?
Conversation
d932125
to
7834804
Compare
cc @ocervell |
a17ec4a
to
2f97e9e
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
0d7b368
to
35a886f
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
pffff whatever. Help me out here, I signed my soul over to the CLA already : ) |
@googlebot I consent. |
35a886f
to
a76a2bc
Compare
closes: google#122 Co-authored-by: Kevin Labesse <[email protected]> Signed-off-by: Kevin Labesse <[email protected]> Signed-off-by: Denis Zaitcev <[email protected]>
a76a2bc
to
804204c
Compare
Hi @oktocat , sorry for your issues with the bot ! I'll review this shortly, quite a big PR :) |
Signed-off-by: Denis Zaitcev <[email protected]>
Signed-off-by: Denis Zaitcev <[email protected]>
3b597a1
to
7c029f5
Compare
@echo "running on Linux, assuming Ubuntu, installing snappy headers with apt" | ||
sudo apt install libsnappy-dev -y || apt install libsnappy-dev -y | ||
endif | ||
$(PIP) install -e ."[api, datadog, prometheus, elasticsearch, pubsub, cloud_monitoring, bigquery, dev, prometheus_remote_write]" |
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'd prefer to keep the install
clean of additional dependencies, can you move this to install_snappy
function and call it after clean
?
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.
Looking great so far, thanks for adding support for this !
One thing is missing: adding the snappy install to the Dockerfile so that Docker installs can work with your exporter: could you please take care of this ?
Please also address the few points and merge the latest master into your branch so that tests can pass before we merge :)
@@ -74,11 +85,11 @@ coverage: | |||
lint: flake8 pylint | |||
|
|||
flake8: | |||
flake8 --ignore=$(FLAKE8_IGNORE) $(NAME)/ --max-line-length=80 | |||
flake8 --ignore=$(FLAKE8_IGNORE) --exclude slo_generator/exporters/gen/ $(NAME)/ --max-line-length=80 |
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 you please move the slo_generator/exporters/gen
folder to slo_generator/proto
so that we can split it from the rest of the exporters code ?
import requests | ||
import snappy | ||
from slo_generator.exporters.base import MetricsExporter | ||
from slo_generator.exporters.gen.remote_pb2 import ( |
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.
ditto, this import will become from slo_generator.proto.remote_pb2 import
after moving the generated files to the proto folder
@oktocat It's unfortunate that this PR has been closed, as we'd rather not deploy prometheus pushgateway just to support slo-generator. Would you have any objections if I continued work on your code? |
@skinlayers feel free to continue this work and open a new PR ;) |
@oktocat @lvaylet
|
Hi @skinlayers, sorry to hear that. I am not aware of recurrent 503 errors on my test setup, but I will take a look. I have been really busy over the last two weeks. Hopefully I can get a look in two days. To this rebasing issue, too. |
@lvaylet Correction: We've been getting 504 timeouts (not 503). I tried increasing the timeout to 1hr, but it didn't seem to help. Since I've seen no one else reporting this issue, I assume its something specific to our setup and customizations.
|
Hi @skinlayers, not sure this is good news but I am also getting a lot of 504 errors after setting up synthetic probes on my CI instance of the SLO Generator. Let me investigate a bit more. |
During testing, I discovered that the error 'Exporter not found in shared config' was caused by proto files for the exporter. If you rebuild them using a new version of protoc and correct the cross imports, then the problem disappears. Linking to the version of the compiler used in prometheus is not necessary: https://github.com/skinlayers/slo-generator/blob/prometheus-remotewrite-exporter/slo_generator/proto/Makefile#L5 I'm not sure that I have enough skills to create a correct PR (rarely work with python). I had to edit the import in the already generated _pb2.py files to make it startup. But I hope that this will help to add this functionality to the main branch. |
closes: #122
Co-authored-by: Kevin Labesse [email protected]
Signed-off-by: Kevin Labesse [email protected]
Signed-off-by: Denis Zaitcev [email protected]