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

Using mime-type in prometheus parser to handle protocol-buffer responses #8545

Merged
merged 4 commits into from
Jan 7, 2021
Merged

Conversation

Aladex
Copy link
Contributor

@Aladex Aladex commented Dec 11, 2020

Fixed #8366

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Just two minor comments. One is in the code, the other one is: Could you please improve the PR-message to e.g. something like "Using mime-type in prometheus parser to handle protocol-buffer responses".

@srebhan
Copy link
Member

srebhan commented Dec 11, 2020

Wow that was fast @Aladex! :-)

@srebhan srebhan self-assigned this Dec 11, 2020
@Aladex Aladex changed the title fixed #8366 Using mime-type in prometheus parser to handle protocol-buffer responses Dec 11, 2020
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@srebhan srebhan added fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Dec 11, 2020
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

Can we get a test to protect against regressions here?

@Aladex
Copy link
Contributor Author

Aladex commented Dec 16, 2020

Looks great. Thank you!

Can we get a test to protect against regressions here?

Maybe, but i don't know how to do it :-(

@srebhan
Copy link
Member

srebhan commented Dec 16, 2020

@Aladex you need one test-data sample in protocol-buffers format. The you start a small http-server to serve this sample to the plugin. Check prometheus_test.go (last test-case) to get an idea on how to do it...

@Aladex
Copy link
Contributor Author

Aladex commented Dec 17, 2020

@Aladex you need one test-data sample in protocol-buffers format. The you start a small http-server to serve this sample to the plugin. Check prometheus_test.go (last test-case) to get an idea on how to do it...

Yes, but here we have examples with classic httptest.NewServer. How do i make such tests with the protobuf server?

@srebhan
Copy link
Member

srebhan commented Dec 17, 2020

Let me understand this, I thought you query a http-server and get back a response with the mime-type set to protocol buffers and the message content is a binary protocol-buffer frame. Is this correct? If so, you can use the httptest.NewServer example, set the response header (e.g. https://www.lemoda.net/go/override-mime-type/index.html) of the answer and embedd serialized protocol-buffer data. The protocol-buffer data you might be able to generate from a real request to a real server offline and then just add the bytes as a variable (or file if it is larger).

@srebhan srebhan added area/prometheus and removed ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Dec 18, 2020
@Aladex
Copy link
Contributor Author

Aladex commented Dec 22, 2020

@srebhan hi. Can u review my latest changes?

@Aladex Aladex requested review from srebhan and ssoroka December 22, 2020 10:42
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks perfect.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Dec 23, 2020
@ssoroka ssoroka merged commit 4b7d113 into influxdata:master Jan 7, 2021
@ssoroka
Copy link
Contributor

ssoroka commented Jan 7, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prometheus fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input.prometheus plugin unable to parse #HELP comment.
3 participants