PureStorage-OpenConnect / pure-fa-openmetrics-exporter

Pure Storage OpenMetrics exporter for FlashArray
Apache License 2.0
20 stars 27 forks source link

Add new URL parameter to API #147

Closed jddarby closed 2 months ago

jddarby commented 3 months ago

This PR aims to reduce confusion by adding a new URL parameter named address.

The idea here is to allow the exporter to clearly understand the intent of the client when it comes to authorization of the FAClient. Currently, the API is somewhat confusing as seen from the conversation in issue #143, with confusion coming from how the exporter uses tokens from tokens.yaml vs tokens from the auth header of the incoming request .

By adding a new URL parameter, the intent of the client is clear:

chrroberts-pure commented 2 months ago

Hi @jddarby , sorry for the delay!

So my concerns with the PR is, is that I mildly agree that the endpoint param can be a little confusing, but we have to ensure backwards compatibility, and we're not currently in a position to release a new major version as that param may changing in the very near future.

With that being said, this approach as currently written isn't going to allow for that goal.

Your comment from #143 makes sense to me "My alternative idea would be that we try to authenticate with the header token and fall back to the token in tokens.yaml if that fails but I prefer the option above."

This is probably how it should of been implemented in the first place. (if i have a token that I received just in time, use it, otherwise use what I know)

Also - I do highly recommended, if you're able to, to update README.md to document this behavior, as also suggested in #143

Please send an email to pure-observability@purestorage.com and I would love to meet via Zoom and give you just a little bit more context.

jddarby commented 2 months ago

Understood. I am closing this PR and I'll open another to implement my fall back option that I gave in #143. I have also sent an email so we can sync up on zoom. I think that once the next PR fixes the problem we will be happy but I'd be interested to have the extra context and hear about how you expect this project to evolve. Thanks!