Skip to content

Add TLS metrics support#175

Merged
ncabatoff merged 1 commit intoncabatoff:masterfrom
dmaganto:feature/add_tls_metrics
Nov 17, 2021
Merged

Add TLS metrics support#175
ncabatoff merged 1 commit intoncabatoff:masterfrom
dmaganto:feature/add_tls_metrics

Conversation

@dmaganto
Copy link
Copy Markdown
Contributor

No description provided.

@SuperQ
Copy link
Copy Markdown
Contributor

SuperQ commented Jan 11, 2021

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

@dmaganto
Copy link
Copy Markdown
Contributor Author

@SuperQ modified to use exporter-toolkit.
Regards

Copy link
Copy Markdown
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.

"print manual")
configPath = flag.String("config.path", "",
"path to YAML config file")
tlsConfigFile = flag.String("web.config", "",
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@BetaFold3
Copy link
Copy Markdown

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

@ncabatoff
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
Contributor Author

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

@ncabatoff
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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