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 TLS metrics support #175

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Add TLS metrics support #175

merged 1 commit into from
Nov 17, 2021

Conversation

dmaganto
Copy link
Contributor

No description provided.

@SuperQ
Copy link
Contributor

SuperQ commented Jan 11, 2021

This should use the prometheus/exporter/toolkit web package.

@dmaganto
Copy link
Contributor Author

@SuperQ modified to use exporter-toolkit.
Regards

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Please also squash the commits.

@@ -306,6 +308,8 @@ func main() {
"print manual")
configPath = flag.String("config.path", "",
"path to YAML config file")
tlsConfigFile = flag.String("web.config", "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tlsConfigFile = flag.String("web.config", "",
tlsConfigFile = flag.String("web.config.file", "",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done squash and change in line 311.

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@tuannh99
Copy link

@SuperQ Hi. How's the plan to merge/release this?

@ncabatoff
Copy link
Owner

I'm open to merging this PR, if I understand correctly what it does, but I'd like to see some doc changes included first.

@dmaganto
Copy link
Contributor Author

dmaganto commented Nov 2, 2021

Sure, @ncabatoff let me do it. This is to enable TLS in the /metrics endpoint in order to allow use this in secure environment.

@lucian-vanghele
Copy link

@dmaganto - I believe @ncabatoff is waiting to update documentation also. Just pointing to Prometheus documentation https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md should be enough I would say.

@dmaganto
Copy link
Contributor Author

Yes, you're right. Do you think that should be enough @ncabatoff?

@ncabatoff
Copy link
Owner

Yeah that would be fine with me, if you mean adding a change to this PR updating the readme to document the new flag and to reference exporter-toolkit's docs re the file format.

@dmaganto
Copy link
Contributor Author

dmaganto commented Nov 16, 2021

@ncabatoff I've added some documentation and reference to get further information to the main project repo. Please take a look.

@ncabatoff ncabatoff merged commit 44d59f9 into ncabatoff:master Nov 17, 2021
@ncabatoff
Copy link
Owner

Thanks!

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.

5 participants