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

Add support to retrieve grain state #360

Merged

Conversation

leonardosimoura
Copy link
Contributor

Added initial support for retrieving a grain's state

The logic is:
Get the methods without parameters in the grain interface
and check if its return type is present in the concrete type of the grain through a property or field of IPersistentState<>, IStorage<>, IStorage

I had to change the TypeFormatter.cs, because for some reason some grains got the wrong name (If needed i can make a separete PR for this one)

If you have any suggestions, please let me know.

Results:
image

image

@SebastianStehle
Copy link
Contributor

SebastianStehle commented Jun 20, 2022

I think we should not have a list of grain IDs and use a list instead. There could be 100.000s of grains.

I also do not understand all this reflection magic, but anyway, I think the grains should provide their state if the feature should be used. The IStorage is an implementation detail and not all grains use the built-in storage mechanism. And what is about in-memory state?

@leonardosimoura
Copy link
Contributor Author

I've been thinking about these other cases and for me the best way would be to create an attribute and mark the desired methods
something like:

[DashboardOutputState]
public async Task<CounterState> GetState()

I believe that we can also maintain this model by reflection, as it is already automatic and the use of IPersistentState is recommended by the official documentation

In the case of listing IDs, it is possible to implement a "virtual view" where only 20 or 50 items are rendered at a time, thus avoiding overloading the browser

@richorama
Copy link
Member

Hey @leonardosimoura it looks like you have put a lot of effort in this work, but I'm sorry to say that the dashboard generally deals with aggregates, rather than providing access to individual grains. It also tries not to require you to make any specific changes to your grain code. However, I see the value in the feature you have developed, but I think this could be better suited to some kind of extension to the dashboard or another project. I would be keen to hear your thoughts and motivations for the work.

@SebastianStehle
Copy link
Contributor

I see it differently.

For me the principles of the dashboard are...

  • Does not require changes to your code.
  • Does not have a big impact to performance.

Other solutions are much better in dealing with aggregates, e.g. Open Telemetry and NewRelic. The dashboard is good to give you an overview of the current state.

If we can achieve this and a feature is useful I would integrate it, but there are two things that needs to be discussed:

  1. How to retrieve state? I would say we should just have a convention. If a grain has a "GetState" method it is used.
  2. How to find grains? For me the list is almost useless. Because of the following reasons:
  • You can have a large number of grains.
  • The list cannot provide any other useful information, e.g. a username, if there would be a user-grain.
  • The grain identity is too hard to use in a list (think about Guid).
  • The list would only return activated grains.
  • The list would be very long.
  • You have too loop over all silos manually or in code to get all your activations.

So for me a simple detail page makes sense, where you enter a grain ID in search bar and you get the information for this grain (and you can activate it if you want).

@leonardosimoura
Copy link
Contributor Author

@richorama
the idea is just a way to see the state of a grain, for now I always need to expose some api/endpoint to achieve this

I believe it is even a resource that will be most used for development and staging environments

a detail page is a good idea

for this PR i think is better just a field for the GraindId and the output of the current state

After this we can create a issue to discuss more about features on this "detail page"

If this is ok to everyone, i can make the changes.

@SebastianStehle
Copy link
Contributor

SebastianStehle commented Jun 23, 2022

It is @richorama decision after all. I would implement it like this:
Page_1

And then just use ace editor for the state.

@richorama
Copy link
Member

ok, yes, I agree that the feature is useful.

I see the UX as something like this:

  1. pick grain type (which may also require a concrete type if there are multiple implementations of the interface)
  2. enter id (could be a compound key)
  3. the grain methods are listed
  4. you can call a method by pressing a button. Perhaps as an MVP we just call methods that don't take any arguments?

I think as a developer it would be a useful way to interact with grains.

I think it should a feature that can be turned on/off.

Thanks you both!

@SebastianStehle
Copy link
Contributor

you can call a method by pressing a button. Perhaps as an MVP we just call methods that don't take any arguments?

I like this.

@koenbeuk
Copy link
Contributor

4. you can call a method by pressing a button. Perhaps as an MVP we just call methods that don't take any arguments?

I actually have this implemented in an internal control panel. I list the grains that are currently active, though you can also address a grain by its interface and grainid. You can then send messages to that grain by interaction with its exposed methods. Parameters are json serialized on the frontend and deserialized in a 'proxy service' that does the work for you. Results are then json serialized and displayed respectively.

Instead of having this in an internal dashboard, I would love to see this feature as a part of the orleans dashboard. It has no runtime overhead except for the listing of active grains which does locking internally in Orleans. I would also be happy to contribute.

This however is not relevant for this PR. Yet we can perhaps continue the discussion here. In general, I think this feature makes a lot of sense and I agree with @SebastianStehle that this is a perfectly reasonable feature of the dashboard (hope I didn't quote you wrong there). Would love to introduce a separate PR (or commit to this PR, once I've gone through it), or otherwise have an extension option within the dashboard such that you can bring this functionality in by choice.

I will consider the above and see what makes sense. Would like input on whether a more generic plugin approach would be preferable compared to a direct PR on this repo.

@leonardosimoura
Copy link
Contributor Author

Hi all
the updates related to state of grains

the support for generic grains will be a little more complicated, for an mvp I chose to ignore them, and do that in a second moment

image

@richorama I liked the idea about interacting with the methods too, I'd just like to leave it to implement in a separate PR.

@koenbeuk wow that's amazing

My idea is to create a dedicated issue for this new “Grain Details” page to us define the desired features.

@leonardosimoura
Copy link
Contributor Author

Do you guys have any other points to adjust or can we finalize the PR?

@SebastianStehle
Copy link
Contributor

Just a few minor things:

  1. Is this not a bootstrap theme? Then we should add the input and btn classes to the elements.
  2. Can we remove the padding of around the second card with the ace editor? Would look better.

@leonardosimoura
Copy link
Contributor Author

@SebastianStehle Changes made

image

@SebastianStehle
Copy link
Contributor

LGTM

@leonardosimoura
Copy link
Contributor Author

@richorama anything else?

Copy link
Member

@richorama richorama left a comment

Choose a reason for hiding this comment

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

This looks like a really nice piece of work! It would be great to switch the ACE editor for something a little more lightweight, I also wonder if there is a way of reducing the complexity of the reflection code, and having some unit tests for that?

App/components/display-grain-state.jsx Outdated Show resolved Hide resolved
OrleansDashboard/Implementation/Grains/DashboardGrain.cs Outdated Show resolved Hide resolved
@leonardosimoura
Copy link
Contributor Author

@richorama improvements made

@@ -23,6 +23,7 @@
"@babel/core": "^7.3.4",
"@babel/preset-env": "^7.3.4",
"@babel/preset-react": "^7.0.0",
"@types/ace": "^0.0.48",
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is causing the npm build to fail.

@richorama richorama merged commit 6f9b8a3 into OrleansContrib:4.0 Jul 26, 2022
@richorama
Copy link
Member

Awesome, thank you!

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.

4 participants