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
Registry log #9829
Registry log #9829
Conversation
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Here's the video clip of the new features I went the thought that people would register some model and if they wanna see the logs of the new model they could refresh the webpage itself rather than restarting the server and will get the quantity of the entities that are registered successfully or not . |
It's send the event to the ui on the first load of the page as well as when the page is refreshed |
@@ -1296,7 +1378,7 @@ func (h *Handler) RegisterMeshmodelComponents(rw http.ResponseWriter, r *http.Re | |||
func (h *Handler) GetMeshmodelRegistrants(rw http.ResponseWriter, r *http.Request) { | |||
rw.Header().Add("Content-Type", "application/json") | |||
enc := json.NewEncoder(rw) | |||
|
|||
registry.Mutex.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MUzairS15 should i use this to sync the registering and fetching of the data i Don't think it is necessary just a fail safe if someone opens the server if the entites are still being registered can you confirm this
meshery/server/meshmodel/helper.go
Line 167 in dd67ed6
meshmodel.Mutex.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not required
@MUzairS15 is there some existing mechanism that we should be using or do we need this new paradigm of storing and displaying log files? |
Good of you to stick with today's extra meeting, @Jougan-0 👍 |
It was fascinating to realize that I haven't yet scratched the surface of the project and still have a lot to learn, for me to be more comfortable and useful in future discussions. |
@@ -162,6 +164,7 @@ func (erh *EntityRegistrationHelper) generateRelationships(pathToComponents stri | |||
// If an error occurs, it logs the error | |||
func (erh *EntityRegistrationHelper) watchComponents(ctx context.Context) { | |||
var err error | |||
meshmodel.Mutex.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These mutex is for shared variables right used in registryLog function?
If so move this lock smd unlock within that function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MUzairS15
but we are locking as the registering is started when it enters the watchComponent for the first time
the main aim of this mutex is that the function GetmeshmodelRegistrant shouldn't be displaying the value if the registration is not complete and unlock it when the registration is complete . That is why we are locking it at the start and then unlocking it when the registration is complete .
https://github.com/meshery/meshery/blob/e2bbe45b50d5acdb9493bab4fbf90b0998b4db36/server/handlers/component_handler.go#L1378
// swagger:route POST /api/meshmodel/nonRegisterEntity | ||
// Handle POST request for registering meshmodel components. | ||
// | ||
// Validate the given value with the given schema | ||
// responses: | ||
// 200:MeshModelHostsWithEntitySummary | ||
|
||
// request body should be json | ||
// request body should be of Host format | ||
|
||
func (handler *Handler) NonRegisterEntity(responseWriter http.ResponseWriter, request *http.Request, _ *models.Preference, user *models.User, provider models.Provider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not according to API design.
@Jougan-0 The functionality that you are defining in the PR is crucial, while the output that we get from the current implementation is nice, the approach is not ideal.
How do you ensure that the registration might have been completed by the time UI makes a request to this endpoint?
My question still stands, if the server is doing the registration why you cannot consolidate all errors and send it all at once?
See the errorChan channel it listens to all the errors encountered, can't you send in more details on this channel model, component and error
or the event
object itself, store it in an array, and call the cancel
function of the passed in context (when registration completes), when you will call the cancel func the ctx.Done case will be called, there you can iterate over that array and add in more details (i.e. summary, x model, y comps failed to register..... )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screencast from 2024-01-09 06-22-53.webm
@MUzairS15 the reason for not using the errorChan is the same I am displaying in the video when the return type of schema error in meshkit is nil the code works fine and these components are not registered but if we return an error such as schema empty the registration breaks I don't know the reason for it but this doesn't make sense i searched but i don't know why we exit quietly when the schema is empty and if we return an error the code breaks so i opt for using the meshkit to store the error and store their in meshkit itself when the entites try to register .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API design is still not accurate, invoking the event from UI is also inaccurate, find a way to send events from the server.
@MUzairS15, fyi, @Jougan-0 made a hard commitment to complete this effort by Friday, Jan 19th. If not completed by then, we'll need to toss in the towel here and carry forward these concerns separately under this issue: #9943 |
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
The tasks are left because i can't figure out how can i send the event directly from the function that has logs the values to the terminal because the user.ID is not present when the make server command is ran. |
Signed-off-by: Jougan-0 <prasantmishra2018@gmail.com>
@Jougan-0 use all zeros for the user ID. |
@leecalcote please correct me if I am wrong here but first the event is created with new event and then we do persistEvent and then we send the using the while setting the id to all 0s nothing is shown in the notification center
I am using this id |
Thanks @Jougan-0 for your effort, but we will not be proceeding with the approach proposed. |
Notes for Reviewers
This PR fixes #9555
Signed commits