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

feat(BOUN-1168): add dynamic route provider (Discovery Library) #568

Conversation

nikolay-komarevskiy
Copy link
Contributor

@nikolay-komarevskiy nikolay-komarevskiy commented Jun 25, 2024

Description

API boundary nodes (BNs) have already been introduced into the Internet Computer (IC) landscape. Currently, the agent does not support dynamic discovery of existing API BNs or the dynamic routing of requests to these nodes based on specific strategies, such as latency. This PR enhances the agent by adding functionality for the dynamic routing to API BNs. In a nutshell the newly introduced DynamicRouteProvider is the implementation of the RouteProvider trait.

Example usage 1 - ergonomic:

#[tokio::main]
async fn main() {
    let client = Client::builder().build().expect("failed to build a client");

    // Initialize the agent with a custom dynamic transport layer
    // NOTE: this requires an async context
    let agent = Agent::builder()
        .with_discovery_transport(client)
        .await
        .build()
        .expect("failed to create an agent");
    //
    // ... use agent
    //

}

Example usage 2 - fully customized:

#[tokio::main]
async fn main() {
    let client = Client::builder().build().expect("failed to build a client");
    let seeds = vec![Node::new("ic0.app").expect("domain is invalid")];
    let snapshot = LatencyRoutingSnapshot::new();
    let route_provider = DynamicRouteProviderBuilder::new(snapshot, seeds, client.clone()).build().await;
    let transport = ReqwestTransport::create_with_client_route(Arc::new(route_provider), client.clone()).unwrap();

    // Initialize the agent with a custom dynamic transport layer
    let agent = Agent::builder()
        .with_transport(transport)
        .build()
        .expect("failed to create an agent");
    //
    // ... use agent
    //

}

How Has This Been Tested?

  1. Added a mainnet test
  2. Created a draft PR and tested on the api_boundary_node_decentralization_test, which is a system-test in the ic-repo.
    NOTE: @adamspofford-dfinity and @lwshang the suggestion to fully remove the fetch_root_key() was probably wrong. As system-test fails without this call. Could You commit a conditional fetch_root_key() invocation, which would work for both mainnet and testnets.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

}

pub fn is_valid_domain<S: AsRef<str>>(domain: S) -> bool {
// TODO

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@nikolay-komarevskiy nikolay-komarevskiy Jun 26, 2024

Choose a reason for hiding this comment

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

i think reusing the Url library is more concise:

pub fn is_valid_domain<S: AsRef<str>>(domain: S) -> bool {
    // Prepend scheme to make it a valid URL
    let url_string = format!("http://{}", domain.as_ref());
    Url::parse(&url_string).is_ok()
}

where
S: RoutingSnapshot + 'static,
{
pub fn new(snapshot: S, seeds: Vec<Node>, http_client: Client) -> Self {

Choose a reason for hiding this comment

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

This means that to create an agent with the discovery library, you need to do something like:

let snapshot = LatencyBasedSnapshot::new();
let http_client = Client::builder().build().unwrap();
let route_provider = HealthCheckRouteProvider::new(
            snapshot,
            vec![Node::new(domain_seed)],
            http_client
);
let transport = ReqwestTransport::create_with_client_route(
        Arc::clone(&route_provider) as Arc<dyn RouteProvider>,
        http_client,
    )
    .expect("failed to create transport");
    
let agent = Agent::builder()
        .with_transport(transport)
        .build()?;

Can't we do something like and just provide defaults:

let agent = Agent::builder()
        .with_latency_based_routing()
        .build()?;

or

let agent = Agent::builder()
        .with_health_based_routing()
        .build()?;

Choose a reason for hiding this comment

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

If someone wants not the default, they can then go through the process above and create their own snapshot/route provider, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually this is the direction to go, yes! But i don't want to change the agent during the library migration phase., better in the follow up.

@nikolay-komarevskiy nikolay-komarevskiy force-pushed the BOUN-1186-add-discovery-library branch 3 times, most recently from 25794d8 to a990a93 Compare June 26, 2024 11:35
@nikolay-komarevskiy nikolay-komarevskiy force-pushed the BOUN-1186-add-discovery-library branch 2 times, most recently from 6ba98ad to 973b377 Compare June 28, 2024 09:52
@nikolay-komarevskiy nikolay-komarevskiy force-pushed the BOUN-1186-add-discovery-library branch 2 times, most recently from d946b18 to ce03d41 Compare July 16, 2024 11:36
@nikolay-komarevskiy nikolay-komarevskiy force-pushed the BOUN-1186-add-discovery-library branch 4 times, most recently from b7492ff to e79769f Compare July 16, 2024 16:10
@nikolay-komarevskiy nikolay-komarevskiy force-pushed the BOUN-1186-add-discovery-library branch 7 times, most recently from cb38300 to d645367 Compare July 17, 2024 18:06
@lwshang lwshang changed the base branch from main to dynamic_route July 18, 2024 17:04
@lwshang lwshang changed the title feat(BOUN-1186): add dynamic route provider (Discovery Library) feat(BOUN-1168): add dynamic route provider (Discovery Library) Jul 19, 2024
@adamspofford-dfinity adamspofford-dfinity merged commit 3010732 into dfinity:dynamic_route Jul 31, 2024
22 checks passed
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.

3 participants