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

remove 'stats' from vatAdmin API #3331

Closed
warner opened this issue Jun 16, 2021 · 2 comments · Fixed by #3337
Closed

remove 'stats' from vatAdmin API #3331

warner opened this issue Jun 16, 2021 · 2 comments · Fixed by #3337
Assignees
Labels
enhancement New feature or request SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Jun 16, 2021

What is the Problem Being Solved?

The vatAdmin~.createVatDynamically() function returns an "admin node" that lets the parent control the new child vat they just spawned. This admin node offers a small number of features:

  • a done() promise that tells you when the vat has terminated
  • a terminateWithFailure() function that lets you kill the vat early
  • an adminData() function that returns "admin statistics" about the vat

The statistics consists of four counters, which measure the number of imported objects, promises, and devices, and the number of deliveries that have been made to the vat.

However, I don't think we've seen a use case for sharing these stats with userspace, and I want the underlying vatKeeper to collect more detailed data (including GC comings and goings), which I'm reluctant to reveal to userspace (and thus include as part of our consensus behavior, which might our options in the future).

So I'm proposing to remove this data from userspace access.

@Chris-Hibbert I think you implemented this originally.. have you encountered a use for the admin stats since then? Any opinions on removing it?

Description of the Design

The "admin node" would remove the adminData() method.

Security Considerations

Less code === more security.

Compatibility Considerations

Better to remove this before promising compatibility.

Test Plan

The unit tests that check for this field (which use `test/vat-admin/bootstrap.js) will have those checks removed. No negative tests will be added (i.e. ones that try to call the method and assert that a TypeError occurred instead of success).

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Jun 16, 2021
@warner warner self-assigned this Jun 16, 2021
@Chris-Hibbert
Copy link
Contributor

I have not seen a use for the functionality.

Once you collect these new stats, where would they be accessible?

I have no indication that the stats I originally added are the right ones to expose to the contract instance creator, they were just the numbers I could easily find. It does seem like contract developers will want performance data so they can improve behavior. We'll have privileged access; will there be any stats others can examine?

If we've thought about it, and not come up with any beneficial uses, I don't have a problem dropping it.

@warner
Copy link
Member Author

warner commented Jun 16, 2021

#3333 has the new stats I'm planning to add. They'd be available from the controller, and by looking at the on-disk database directly, but not from userspace. Developers would probably be best-off getting them from a block explorer application of some sort, rather than allowing a vat itself to retrieve them. One of those things you must get by standing outside the system, rather than being visible from inside.

warner added a commit that referenced this issue Jun 16, 2021
The "Admin Node" returned by `vatAdmin~.createVatDynamically()` had a method
named `adminData()`, which returned some statistics like the number of
objects imported into the vat. This wasn't super-useful and exposes a little
too much information about the kernel's internals, especially as GC starts to
get more interesting. So we've decided to remove this API.

closes #3331
warner added a commit that referenced this issue Jun 16, 2021
The "Admin Node" returned by `vatAdmin~.createVatDynamically()` had a method
named `adminData()`, which returned some statistics like the number of
objects imported into the vat. This wasn't super-useful and exposes a little
too much information about the kernel's internals, especially as GC starts to
get more interesting. So we've decided to remove this API.

closes #3331
warner added a commit that referenced this issue Jun 16, 2021
The "Admin Node" returned by `vatAdmin~.createVatDynamically()` had a method
named `adminData()`, which returned some statistics like the number of
objects imported into the vat. This wasn't super-useful and exposes a little
too much information about the kernel's internals, especially as GC starts to
get more interesting. So we've decided to remove this API.

closes #3331
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants