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

[FINAL] feat: Add sections for new subnet_info endpoint #351

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

michael-weigelt
Copy link

@michael-weigelt michael-weigelt commented Oct 21, 2024

In certain circumstances, canisters wish to learn about subnet-specific data or metadata. This endpoint enables these use cases, starting with exposing the replica version a subnet is currently running.

Open questions:

  • Should this method be restricted to canisters or can any principal call it? Update: Yes, to pass on the cost to the caller
  • Naming: Is 'stats' fine if we anticipate that data like number of canisters, memory occupied etc might be added to the response struct? Update: Renamed to subnet_metrics Update Renaming to subnet_info!

@michael-weigelt michael-weigelt requested a review from a team as a code owner October 21, 2024 13:38
Copy link

github-actions bot commented Oct 21, 2024

🤖 Here's your preview: https://pjpim-riaaa-aaaak-qckja-cai.icp0.io/docs

Copy link
Contributor

@mraszyk mraszyk left a comment

Choose a reason for hiding this comment

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

LGTM

@adambratschikaye
Copy link
Contributor

If a canister wants to get the information for it's own subnet, how is it supposed to figure out which subnet it is on?

@mraszyk
Copy link
Contributor

mraszyk commented Oct 24, 2024

If a canister wants to get the information for it's own subnet, how is it supposed to figure out which subnet it is on?

The registry has an endpoint that serves this information.

github-merge-queue bot pushed a commit to dfinity/ic that referenced this pull request Oct 28, 2024
…lable to execution environment (#2248)

This is an alternative to #2248 and
#2082

This PR passes the running replica version to the execution environment,
from where it can be used [in a future management canister
call.](dfinity/interface-spec#351):
#2202

---------

Co-authored-by: Michael Weigelt <[email protected]>
github-merge-queue bot pushed a commit to dfinity/ic that referenced this pull request Oct 28, 2024
…lable to execution environment (#2248)

This is an alternative to #2248 and
#2082

This PR passes the running replica version to the execution environment,
from where it can be used [in a future management canister
call.](dfinity/interface-spec#351):
#2202

---------

Co-authored-by: Michael Weigelt <[email protected]>
aterga pushed a commit to dfinity/ic that referenced this pull request Oct 29, 2024
…lable to execution environment (#2248)

This is an alternative to #2248 and
#2082

This PR passes the running replica version to the execution environment,
from where it can be used [in a future management canister
call.](dfinity/interface-spec#351):
#2202

---------

Co-authored-by: Michael Weigelt <[email protected]>
@dsarlis
Copy link
Member

dsarlis commented Oct 29, 2024

Is there a reason why we went with subnet_metrics instead of subnet_metadata or subnet_info? The replica_version is not exactly a metric. Do we envision that we would eventually have things like number of canisters or state of the subnet served through this endpoint? Even then, I think subnet_info would still be ok (subnet_metrics would still suffer from including the replica_version which is not really a metric).

@michael-weigelt
Copy link
Author

Do we envision that we would eventually have things like number of canisters or state of the subnet served through this endpoint

Yes, I believe. But I see the point. It's a misnomer now, and will be less of a misnomer later, but still not quite right.

why

Just in case you missed the collapsed item.

@mraszyk
Copy link
Contributor

mraszyk commented Oct 30, 2024

Is there a reason why we went with subnet_metrics instead of subnet_metadata or subnet_info? The replica_version is not exactly a metric. Do we envision that we would eventually have things like number of canisters or state of the subnet served through this endpoint? Even then, I think subnet_info would still be ok (subnet_metrics would still suffer from including the replica_version which is not really a metric).

I agree that subnet_info makes more sense and is consistent with the existing canister_info endpoint. We already have a path /subnet/<subnet_id>/metrics in the state tree though and if we eventually converge into having the same data in the state tree and in subnet_info, then the name would be inconsistent (unless we could rename the state tree path).

@dsarlis
Copy link
Member

dsarlis commented Oct 30, 2024

Is there a reason why we went with subnet_metrics instead of subnet_metadata or subnet_info? The replica_version is not exactly a metric. Do we envision that we would eventually have things like number of canisters or state of the subnet served through this endpoint? Even then, I think subnet_info would still be ok (subnet_metrics would still suffer from including the replica_version which is not really a metric).

I agree that subnet_info makes more sense and is consistent with the existing canister_info endpoint. We already have a path /subnet/<subnet_id>/metrics in the state tree though and if we eventually converge into having the same data in the state tree and in subnet_info, then the name would be inconsistent (unless we could rename the state tree path).

The state tree path contains only metrics for now so that is consistent imo. If we want to unify at some point (the two return a different set of information for now), then we can switch to subnet_info for the state tree path too. (Likely by introducing a new path that supersedes the old for backward compatibility).

@Dfinity-Bjoern
Copy link
Member

I am also fine with subnet_info, if that's the preferred term overall.

@michael-weigelt
Copy link
Author

Thanks for the changes @mraszyk. Do we consider this PR final? @Dfinity-Bjoern
The implementation is updated, here.

@Dfinity-Bjoern Dfinity-Bjoern changed the title feat: Add sections for new subnet_stats endpoint [FINAL] feat: Add sections for new subnet_info endpoint Nov 1, 2024
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