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

Admin UI and minor fixes #469

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

Admin UI and minor fixes #469

wants to merge 55 commits into from

Conversation

marsian83
Copy link
Collaborator

Marked as draft for now

marsian83 and others added 30 commits October 24, 2023 01:04
Comment on lines +1 to +2
import axios, { AxiosRequestConfig } from "axios";
import { useEffect, useState } from "react";
Copy link
Member

Choose a reason for hiding this comment

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

Here too

Comment on lines +1 to +2
import { useState, useEffect } from "react";

Copy link
Member

Choose a reason for hiding this comment

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

Header

Comment on lines +9 to +15
const benchmarkWorkloads = [
"OLTP-SET",
"OLTP-READONLY",
"OLTP-READONLY-OLAP",
"TPCC",
"OLTP",
];
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this list dynamic based on the list of benchmarks returned by the API?

Copy link

Choose a reason for hiding this comment

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

Can we make this list dynamic based on the list of benchmarks returned by the API?

I thnk I know how to do this

func (s *Server) getBenchmarkNames(c *gin.Context) {
	c.JSON(http.StatusOK, gin.H{
		"benchmarks": s.benchmarkTypes,
	})
}

This code snippet if added in go/server and adding a respective route like /api/get-benchmark-types
Will give list which we can use in frontend.
@frouioui

Comment on lines +1 to +2

export default function AdminPanelPage() {
Copy link
Member

Choose a reason for hiding this comment

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

Header is missing here too

Comment on lines +1 to +2
import { Link } from "react-router-dom";

Copy link
Member

Choose a reason for hiding this comment

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

Header here too :)

@@ -0,0 +1,87 @@
import React, { useEffect, useState } from "react";
Copy link
Member

Choose a reason for hiding this comment

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

header missing

Comment on lines +2 to +3
// import api from "../../services/api";

Copy link
Member

Choose a reason for hiding this comment

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

let's remove this

Comment on lines +1 to +2
import Macrobench from "../common/Macrobench";
import { BenchmarkStatus } from "./enums";
Copy link
Member

Choose a reason for hiding this comment

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

header

Comment on lines +85 to +86
export interface Macros {
OLTP: MacroBenchmark[] | null;
Copy link
Member

Choose a reason for hiding this comment

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

same thing here as mentioned above, would it be possible to dynamically load the name of the macro benchmarks? the list of macro benchmark evolve over time

@codingmickey
Copy link

Hey @frouioui @marsian83 does this need any help, I'd be happy to take this up and bring it to at least a completion!

@frouioui
Copy link
Member

frouioui commented May 9, 2024

@codingmickey Hello! I don't think @marsian83 has planned on continuing this given there was no response for months. You're welcome to take this on!

@marsian83
Copy link
Collaborator Author

Hey @frouioui @marsian83 does this need any help, I'd be happy to take this up and bring it to at least a completion!

Hi, it'd be great if you could honestly.
The majority of the issues currently are due to mismatch of the type declarations we have on the frontend side vs the responses we recieve from the server.
I'd recommend a straigh forward approach of checking the response types for the api calls in the errored pages and changing around the types accordingly.
If you need any help understanding anything do let me know.

@marsian83
Copy link
Collaborator Author

Hi @codingmickey
I see there are a lot of conflicts with the current state of the HEAD and the main
let me resolve these conflicts first before you could look into this
Mind waiting a day or two?

@codingmickey
Copy link

codingmickey commented May 11, 2024 via email

- Macrobench
- Daily
- Compare
@harry-sandhu
Copy link

Hi, I made progress for this
I fix some pages, where api had changed so I changed the api.ts file
and fixed the rendering methods where the api response was being used
But I did that on @marsian83, your fork's head
but I can not push

@frouioui @marsian83 Can you please guide me aroudn this github issue?

@marsian83
Copy link
Collaborator Author

Hi, I made progress for this I fix some pages, where api had changed so I changed the api.ts file and fixed the rendering methods where the api response was being used But I did that on @marsian83, your fork's head but I can not push

@frouioui @marsian83 Can you please guide me aroudn this github issue?

You can fork my branch and then create PR to it, when I merge those, it will be added here.
have you already made commits?

@harry-sandhu
Copy link

I have fixed all pages
Daily page is not working, the endpoint returns null
the endpoint /api/daily even the production version https://benchmark.vitess.io/api/daily returns null
Please help me with this @frouioui @Camillemtd @marsian83
Also, I request you to test and review
.Thanks

@@ -138,73 +138,73 @@ export default function Macrobench(props: MacrobenchProps) {
left={fixed(data.diff.Left.Result.threads, 2)}
right={fixed(data.diff.Right.Result.threads, 2)}
diffMetric={fixed(data.diff.Diff.threads, 2)}
/>
/> */}
Copy link
Collaborator Author

@marsian83 marsian83 May 26, 2024

Choose a reason for hiding this comment

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

Why have you commented out these sections?

Choose a reason for hiding this comment

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

the response returned by api does not have specific values that correspond to these .This is the reason they are commented out.

@@ -23,6 +23,7 @@ export default function useApiCall<T extends ApiEndpoint>(
},
...config,
});
console.log(`Response from ${uri}:`, response.data);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please remove any unused / unnecessary console logs

@@ -20,11 +20,11 @@ import PropTypes from "prop-types";
import { fixed, formatByteForGB } from "../../../utils";
import { Link } from "react-router-dom";

export default function SearchMacro({ data, gitRef }: any) {
export default function SearchMacro({ data, macroName, gitRef }: any) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you figure out how to do this without explicitly using any

@marsian83
Copy link
Collaborator Author

I have reviewed your code @harry-sandhu
Testing seems to indicate all pages work fine, except of course the daily page.
I will see what can be wrong with the /api/daily endpoint

marsian83 and others added 4 commits May 26, 2024 15:08
Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com>
Signed-off-by: Spandan Barve <114365550+marsian83@users.noreply.github.com>
Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com>
Signed-off-by: Spandan Barve <114365550+marsian83@users.noreply.github.com>
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.

None yet

4 participants