Auto-configured PrometheusPushGatewayManager cannot be configured to use HTTPS to connect to the gateway #16084
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
|
||
| private PushGateway getPushGateway(String url) { | ||
| try { | ||
| return new PushGateway(new URL(Objects.toString(url, ""))); |
There was a problem hiding this comment.
I don't think we need to do Objects.toString(url, "") since the property has a default value of localhost:9091.
wilkinsona
changed the title
|
@mbhave Thank you very much! |
There was a problem hiding this comment.
Thanks for the PR.
I am not sure that's quite what we had in mind. We want to get away from using the String based constructor as I am expecting prometheus to deprecate and remove it in a future release. We also want to educate our users and offer a migration strategy.
I am not sure how to deal with this: the property is called base-url but isn't an URL so we can't just change its type. If we keep a String there is no easy way to signal to users they have to provide a valid URL. IDEs do not perform any validation on the String at the moment so using a custom value hint won't help.
Push gateway calls the string-based one an address but I guess it's a bit dumb to rename it to re-introduce the URL-based afterwards.
Flagging for team attention to see what the rest of the team thinks.
| Duration pushRate = properties.getPushRate(); | ||
| String job = getJob(properties, environment); | ||
| Map<String, String> groupingKey = properties.getGroupingKey(); | ||
| ShutdownOperation shutdownOperation = properties.getShutdownOperation(); | ||
| return new PrometheusPushGatewayManager(pushGateway, collectorRegistry, | ||
| pushRate, job, groupingKey, shutdownOperation); | ||
| return new PrometheusPushGatewayManager( |
There was a problem hiding this comment.
I don't see a reason to change the flow of the code here. I also feel that the previous version was easier to read. Can you please revert that part?
There was a problem hiding this comment.
spring-java-format :(
I've introduced a variable to avoid this.
There was a problem hiding this comment.
That probably wasn't what I was talking about. I meant the fact you remove the PushGateway variable and therefore changed this constructor. I see no good reason for doing so.
There was a problem hiding this comment.
There are 2 constructors.
StringcreatesURLas"http://" + addressand delegates it to the underlyingURLconstructor.URLusesURLas is.
So, that is why I've introduced a method to create a PushGateway instance using different constructors.
There was a problem hiding this comment.
That's not what I am talking about and what you pushed force in the meantime is addressing it, thank you.
…use HTTPS to connect to the gateway. spring-projectsgh-16073
snicoll
added
the
for: team-attention
snicoll
removed
the
for: team-attention
|
We've decided to try to parse the URL and log a warning if the URL is invalid, calling the deprecated constructor in that case. We'll also change the default value to include |
|
@snicoll Thank you |
* pr/16084: Polish "Permit use of https for configuring Prometheus push gateway" Permit use of https for configuring Prometheus push gateway
gh-16073