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

Implement am proxy command #128

Merged
merged 13 commits into from
Aug 30, 2023
Merged

Implement am proxy command #128

merged 13 commits into from
Aug 30, 2023

Conversation

brettimus
Copy link
Contributor

@brettimus brettimus commented Aug 28, 2023

Description

This PR adds support for an am proxy command, which would

  1. Start serving explorer, as usual (can still be configured via --listen-address and --explorer-address)
  2. Optionally proxy requests to /prometheus to a user-specified url (configured via --prometheus-url)
  3. NOT download or launch a local Prometheus

So, if we already had prometheus running on http://localhost:8063 for some odd reason, we could run:

am proxy --prometheus-url http://localhost:8063

And am would not download/launch prometheus, but explorer would still be accessible and automagically connected to the Prometheus instance we're interested in connecting to.

For what it's worth, I tested that scenario, and it does indeed work.

Where I need help

I wanted to create a handler factory, but struggled immensely with the type definition, because you can't use impl Trait to describe the return type for a Fn trait (rust-lang/rust#99697).

What I wanted was something that looked like:

if is_proxying_prometheus {
        app = app
            .route("/prometheus/*path", any(prometheus::handler_factory(prometheus_upstream_base)))
            .route("/prometheus",  any(prometheus::handler_factory(prometheus_upstream_base)));
    }

Ultimately, I gave up, and opted to use inline blocks/closures to define the handlers for this functionality.

Additionally, I figured we would need to rewrite the path of any requests to /prometheus/* before forwarding to the external prometheus. There has to be a better/more concise way than I chose.

Basically, this PR still needs some cleanup, api review (is proxy the right command name even?), and probably some tests.

Checklist

  • Changelog updated

@brettimus brettimus requested a review from a team as a code owner August 28, 2023 16:01
@brettimus brettimus marked this pull request as draft August 28, 2023 16:01
@brettimus brettimus marked this pull request as ready for review August 28, 2023 20:32
@hatchan
Copy link
Contributor

hatchan commented Aug 30, 2023

Additionally, I figured we would need to rewrite the path of any requests to /prometheus/* before forwarding to the external prometheus. There has to be a better/more concise way than I chose.

I think this is the correct way of dealing with this 👍

Basically, this PR still needs some cleanup, api review (is proxy the right command name even?), and probably some tests.

I think proxy is the best name.

.gitignore Outdated Show resolved Hide resolved
src/bin/am/commands/proxy.rs Outdated Show resolved Hide resolved
src/bin/am/server.rs Outdated Show resolved Hide resolved
brettimus and others added 5 commits August 30, 2023 10:51
Co-authored-by: Benno van den Berg <hatchan@users.noreply.github.com>
Co-authored-by: Benno van den Berg <hatchan@users.noreply.github.com>
@brettimus brettimus changed the title [WIP] am proxy Implement am proxy command Aug 30, 2023
@brettimus brettimus merged commit 7e7bebf into main Aug 30, 2023
1 check passed
@brettimus brettimus deleted the am-proxy branch August 30, 2023 14:47
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.

2 participants