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

Add HTTPS configuration #709

Closed
wants to merge 2 commits into from
Closed

Conversation

replay
Copy link

@replay replay commented Jan 28, 2021

This PR is based on #693

Add the exporter-toolkit https package to allow configuring TLS and
auth.

Signed-off-by: Ben Kochie <[email protected]>
@replay replay marked this pull request as ready for review January 28, 2021 21:25
@replay replay requested a review from a team as a code owner January 28, 2021 21:25
Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

I'm also very keen to see HTTPS support in the windows_exporter, but the use of two logging libraries will cause confusion for users.
Those running the exporter from the MSI install will have most logs sent to Windows Event Log, while HTTPS-related events sent to STDOUT will be effectively dropped.

It's not a problem with an easy solution, as the exporter-toolkit/https library requires go-kit/kit/log which doesn't currently support Windows Event logging.

@replay
Copy link
Author

replay commented Jan 29, 2021

I'm also very keen to see HTTPS support in the windows_exporter, but the use of two logging libraries will cause confusion for users.
Those running the exporter from the MSI install will have most logs sent to Windows Event Log, while HTTPS-related events sent to STDOUT will be effectively dropped.

That's definitely not optimal

It's not a problem with an easy solution, as the exporter-toolkit/https library requires go-kit/kit/log which doesn't currently support Windows Event logging.

Afaict we have the following options:

  1. Use mixed loggers, which is what this PR implements, you have already pointed out the disadvantages of this.
  2. Use a different HTTPS server than the one from the exporter-toolkit, that's what I implemented in this PR.
  3. Update the exporter-toolkit and make its HTTPS server use the logger from github.com/prometheus/common/log, I don't know enough about that library to judge how difficult/easy that would be and whether such a change would get accepted.

@carlpett
Copy link
Collaborator

What I've done on the half-finished work that I mentioned in the other PR is a fourth option, namely a wrapper. I can try to get that finished tonight, since there appears to be much interest in this.

@replay
Copy link
Author

replay commented Feb 19, 2021

Closing this because #693 has been merged

@replay replay closed this Feb 19, 2021
@replay replay deleted the https2 branch February 19, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants