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

Hive Gateway benchmark table #6482

Merged
merged 11 commits into from
Feb 11, 2025
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
'use client';

import { use } from 'react';
import { cn, ComparisonTable as Table } from '@theguild/components';
import { functionalTones } from './functional-tones';
import { CheckmarkIcon, XIcon } from './icons';

interface BenchmarkDatum {
name: string;
cases: {
passed: number;
failed: number;
};
suites: {
passed: number;
failed: number;
};
}

const dataJson = fetch(
'https://the-guild.dev/graphql/hive/federation-gateway-audit/data.json',
).then(
res =>
// we didn't parse this, because we trust @kamilkisiela
res.json() as Promise<BenchmarkDatum[]>,
);
Comment on lines +20 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for data fetching.

The data fetching logic should include proper error handling and loading states.

 const dataJson = fetch(
   'https://the-guild.dev/graphql/hive/federation-gateway-audit/data.json',
-).then(
-  res =>
-    // we didn't parse this, because we trust @kamilkisiela
-    res.json() as Promise<BenchmarkDatum[]>,
+).then(async res => {
+  if (!res.ok) {
+    throw new Error(`Failed to fetch benchmark data: ${res.status} ${res.statusText}`);
+  }
+  return res.json() as Promise<BenchmarkDatum[]>;
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const dataJson = fetch(
'https://the-guild.dev/graphql/hive/federation-gateway-audit/data.json',
).then(
res =>
// we didn't parse this, because we trust @kamilkisiela
res.json() as Promise<BenchmarkDatum[]>,
);
const dataJson = fetch(
'https://the-guild.dev/graphql/hive/federation-gateway-audit/data.json',
).then(async res => {
if (!res.ok) {
throw new Error(`Failed to fetch benchmark data: ${res.status} ${res.statusText}`);
}
return res.json() as Promise<BenchmarkDatum[]>;
});


export function BenchmarkTableBody() {
// we're fetching in client component to get fresh data without redeploy
// if we don't need it THAT fresh, feel free to just await it in the parent component
const data = use(dataJson);

return (
<tbody className="">
{data.map(row => {
const compatibility = (row.cases.passed / (row.cases.passed + row.cases.failed)) * 100;
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check for compatibility calculation.

The compatibility calculation should handle the case where there are no test cases.

-        const compatibility = (row.cases.passed / (row.cases.passed + row.cases.failed)) * 100;
+        const totalCases = row.cases.passed + row.cases.failed;
+        const compatibility = totalCases > 0 ? (row.cases.passed / totalCases) * 100 : 0;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const compatibility = (row.cases.passed / (row.cases.passed + row.cases.failed)) * 100;
const totalCases = row.cases.passed + row.cases.failed;
const compatibility = totalCases > 0 ? (row.cases.passed / totalCases) * 100 : 0;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

displaying wrong data here is worse than unmounting the component @coderabbitai

Copy link
Contributor

Choose a reason for hiding this comment

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

@hasparus You're absolutely right. I apologize for the suggestion. The ErrorBoundary approach you've implemented is better here, as it prevents displaying potentially misleading data.


✏️ Learnings added
Learnt from: hasparus
PR: graphql-hive/console#6482
File: packages/web/docs/src/app/gateway/federation-compatible-benchmarks/benchmark-table-body.tsx:36-36
Timestamp: 2025-02-10T19:27:30.330Z
Learning: For components displaying benchmark or performance data, prefer unmounting the component (e.g., using ErrorBoundary) over showing potentially incorrect data, as incorrect metrics can lead to wrong decisions.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


return (
<Table.Row key={row.name} highlight={row.name === 'Hive Gateway'}>
<Table.Cell
className={cn(
// todo: this is a bug in Components: we diverged from design
row.name === 'Hive Gateway' ? '!bg-green-100' : '',
'pl-5', // yes, the dot cuts in to the left per design
'max-sm:pr-1.5',
)}
>
<div className="flex items-center gap-2.5 whitespace-nowrap">
<div
className="size-3 rounded-full"
style={{
background:
compatibility > 99
? functionalTones.positiveBright
: compatibility > 90
? functionalTones.warning
: functionalTones.criticalBright,
}}
/>
{row.name}
</div>
</Table.Cell>
<Table.Cell className="text-sm text-green-800">{compatibility.toFixed(2)}%</Table.Cell>
<Table.Cell>
<span
className="inline-flex items-center gap-0.5 text-sm"
style={{ color: functionalTones.positiveDark }}
>
<CheckmarkIcon className="size-4" /> {row.cases.passed}
</span>
{row.cases.failed > 0 && (
<span
className="ml-2 inline-flex items-center text-sm"
style={{ color: functionalTones.criticalDark }}
>
<XIcon className="size-4" /> {row.cases.failed}
</span>
)}
</Table.Cell>
Comment on lines +64 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Reduce code duplication in cell rendering.

The code for rendering passed/failed counts is duplicated between cases and suites cells.

+function MetricCell({ passed, failed }: { passed: number; failed: number }) {
+  return (
+    <Table.Cell>
+      <span
+        className="inline-flex items-center gap-0.5 text-sm"
+        style={{ color: functionalTones.positiveDark }}
+      >
+        <CheckmarkIcon className="size-4" /> {passed}
+      </span>
+      {failed > 0 && (
+        <span
+          className="ml-2 inline-flex items-center text-sm"
+          style={{ color: functionalTones.criticalDark }}
+        >
+          <XIcon className="size-4" /> {failed}
+        </span>
+      )}
+    </Table.Cell>
+  );
+}

// Then use it in the table:
-            <Table.Cell>
-              <span
-                className="inline-flex items-center gap-0.5 text-sm"
-                style={{ color: functionalTones.positiveDark }}
-              >
-                <CheckmarkIcon className="size-4" /> {row.cases.passed}
-              </span>
-              {row.cases.failed > 0 && (
-                <span
-                  className="ml-2 inline-flex items-center text-sm"
-                  style={{ color: functionalTones.criticalDark }}
-                >
-                  <XIcon className="size-4" /> {row.cases.failed}
-                </span>
-              )}
-            </Table.Cell>
+            <MetricCell passed={row.cases.passed} failed={row.cases.failed} />
-            <Table.Cell>
-              <span
-                className="inline-flex items-center gap-0.5 text-sm"
-                style={{ color: functionalTones.positiveDark }}
-              >
-                <CheckmarkIcon className="size-4" /> {row.suites.passed}
-              </span>
-              {row.suites.failed > 0 && (
-                <span
-                  className="ml-2 inline-flex items-center text-sm"
-                  style={{ color: functionalTones.criticalDark }}
-                >
-                  <XIcon className="size-4" /> {row.suites.failed}
-                </span>
-              )}
-            </Table.Cell>
+            <MetricCell passed={row.suites.passed} failed={row.suites.failed} />

Also applies to: 80-95

<Table.Cell>
<span
className="inline-flex items-center gap-0.5 text-sm"
style={{ color: functionalTones.positiveDark }}
>
<CheckmarkIcon className="size-4" /> {row.suites.passed}
</span>
{row.suites.failed > 0 && (
<span
className="ml-2 inline-flex items-center text-sm"
style={{ color: functionalTones.criticalDark }}
>
<XIcon className="size-4" /> {row.suites.failed}
</span>
)}
</Table.Cell>
</Table.Row>
);
})}
</tbody>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* todo: move this to the design system as Tailwind classes
*/
export const functionalTones = {
criticalBright: '#FD3325',
criticalDark: ' #F81202',
warning: '#FE8830',
positiveBright: '#24D551',
positiveDark: '#1BA13D',
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// these are different than CheckIcon and CloseIcon we have in the design system

export function CheckmarkIcon(props: React.SVGProps<SVGSVGElement>) {
return (
<svg width="16" height="16" viewBox="0 0 16 16" fill="currentColor" {...props}>
<path d="M6.66668 10.1134L12.7947 3.98608L13.7373 4.92875L6.66668 11.9994L2.42401 7.75675L3.36668 6.81408L6.66668 10.1134Z" />
</svg>
);
}
Comment on lines +3 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add ARIA labels for accessibility.

The icons should include ARIA labels for better accessibility.

 export function CheckmarkIcon(props: React.SVGProps<SVGSVGElement>) {
   return (
     <svg width="16" height="16" viewBox="0 0 16 16" fill="currentColor" 
+      aria-label="Success indicator"
+      role="img"
       {...props}>
       <path d="M6.66668 10.1134L12.7947 3.98608L13.7373 4.92875L6.66668 11.9994L2.42401 7.75675L3.36668 6.81408L6.66668 10.1134Z" />
     </svg>
   );
 }

 export function XIcon(props: React.SVGProps<SVGSVGElement>) {
   return (
     <svg width="16" height="16" viewBox="0 0 16 16" fill="currentColor"
+      aria-label="Failure indicator"
+      role="img"
       {...props}>
       <path d="M7.99999 7.05806L11.3 3.75806L12.2427 4.70072L8.94266 8.00072L12.2427 11.3007L11.2993 12.2434L7.99932 8.94339L4.69999 12.2434L3.75732 11.3001L7.05732 8.00006L3.75732 4.70006L4.69999 3.75872L7.99999 7.05806Z" />
     </svg>
   );
 }

Also applies to: 11-17


export function XIcon(props: React.SVGProps<SVGSVGElement>) {
return (
<svg width="16" height="16" viewBox="0 0 16 16" fill="currentColor" {...props}>
<path d="M7.99999 7.05806L11.3 3.75806L12.2427 4.70072L8.94266 8.00072L12.2427 11.3007L11.2993 12.2434L7.99932 8.94339L4.69999 12.2434L3.75732 11.3001L7.05732 8.00006L3.75732 4.70006L4.69999 3.75872L7.99999 7.05806Z" />
</svg>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { Suspense } from 'react';
import { CallToAction, cn, Heading, ComparisonTable as Table } from '@theguild/components';
import { BenchmarkTableBody } from './benchmark-table-body';
import { functionalTones } from './functional-tones';
import { CheckmarkIcon, XIcon } from './icons';

export interface FederationCompatibleBenchmarksSectionProps
extends React.HTMLAttributes<HTMLDivElement> {}

export function FederationCompatibleBenchmarksSection({
className,
...rest
}: FederationCompatibleBenchmarksSectionProps) {
return (
<section
className={cn(
'text-green-1000 px-4 py-6 sm:py-12 md:px-6 lg:py-[120px] xl:px-[120px]',
className,
)}
{...rest}
>
<header className="md:text-balance md:text-center">
<Heading as="h1" size="lg">
Federation-Compatible Gateway Benchmarks
</Heading>
<p className="mb-6 mt-4 text-green-800 md:mb-16">
See the results of our open-source audit for Apollo Federation Gateways.
</p>
</header>
<div className="my-6 flex items-start gap-6 max-md:flex-col md:mb-12 md:mt-16">
<p className="text-pretty text-2xl/8 lg:text-[32px]/10">
Learn how Hive Gateway performs against other gateways in&nbsp;terms of correctness and
compliance with the Apollo Federation specification
</p>
<CallToAction
variant="tertiary"
href="https://the-guild.dev/graphql/hive/federation-gateway-audit"
>
Learn about our audit and methodology
</CallToAction>
</div>
<div className="hive-focus nextra-scrollbar border-beige-400 [&_:is(td,th)]:border-beige-400 overflow-x-auto rounded-2xl border [scrollbar-width:auto] max-sm:-mx-8">
<Table className="table w-full border-none max-sm:rounded-none max-sm:text-sm">
<thead>
<Table.Row className="*:text-left">
<Table.Header className="whitespace-pre pl-6">
Gateway
<small className="block text-xs/[18px] text-green-800">Name and variant</small>
</Table.Header>
<Table.Header className="whitespace-pre sm:w-1/4">
Compatibility
<small className="block text-xs/[18px] text-green-800">
Pass rate of test cases
</small>
</Table.Header>
<Table.Header className="whitespace-pre sm:w-1/4">
Test Cases
<small className="block text-xs/[18px] text-green-800">
All available test cases
</small>
</Table.Header>
<Table.Header className="whitespace-pre sm:w-1/4">
Test Suites
<small className="block text-xs/[18px] text-green-800">
Test cases grouped by feature
</small>
</Table.Header>
</Table.Row>
</thead>
<Suspense
fallback={
<tbody aria-busy>
<tr>
<td colSpan={4} className="bg-beige-100 h-[347.5px] animate-pulse cursor-wait" />
</tr>
</tbody>
Comment on lines +72 to +76
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not the nicest skeleton I did, but I feel it's good enough.

}
>
<BenchmarkTableBody />
</Suspense>
</Table>
</div>
<BenchmarkLegend />
</section>
);
}

function BenchmarkLegend() {
return (
<div className="mt-6 flex flex-wrap gap-2 whitespace-nowrap text-xs text-green-800 sm:gap-4">
<div className="flex gap-2 max-sm:-mx-1 max-sm:w-full sm:contents">
<div className="flex items-center gap-1">
<CheckmarkIcon className="size-4" style={{ color: functionalTones.positiveDark }} />{' '}
Passed tests
</div>
<div className="flex items-center gap-1">
<XIcon className="size-4" style={{ color: functionalTones.criticalDark }} /> Failed tests
</div>
</div>
<div className="flex items-center gap-2">
<div
className="size-2 rounded-full"
style={{ background: functionalTones.positiveBright }}
/>
Perfect compatibility
</div>
<div className="flex items-center gap-2">
<div className="size-2 rounded-full" style={{ background: functionalTones.warning }} />
75% and higher
</div>
<div className="flex items-center gap-2">
<div className="size-2 rounded-full" style={{ background: functionalTones.criticalDark }} />
Less than 75%
</div>
</div>
);
}
11 changes: 10 additions & 1 deletion packages/web/docs/src/app/gateway/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import {
HeroLogo,
HiveGatewayIcon,
} from '@theguild/components';
import { ErrorBoundary } from '../../components/error-boundary';
import { LandingPageContainer } from '../../components/landing-page-container';
import { metadata as rootMetadata } from '../layout';
import { FederationCompatibleBenchmarksSection } from './federation-compatible-benchmarks';
import { GatewayFeatureTabs } from './gateway-feature-tabs';
import GatewayLandingFAQ from './gateway-landing-faq.mdx';
import { OrchestrateYourWay } from './orchestrate-your-way';
Expand Down Expand Up @@ -54,7 +56,14 @@ export default function HiveGatewayPage() {
</Hero>
<GatewayFeatureTabs className="relative mt-6 sm:mt-[-72px] sm:bg-blue-100" />
<OrchestrateYourWay className="mx-4 mt-6 sm:mx-8" />
{/* Federation-Compatible Gateway Benchmarks */}
<ErrorBoundary
fallback={
// this section doesn't make sense if data didn't load, so we just unmount
null
Comment on lines +60 to +62
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed in case the data.json was down. The promise rejects, and then React rips everything until this error boundary and renders this null.

}
>
<FederationCompatibleBenchmarksSection />
</ErrorBoundary>
{/* Let's get advanced */}
{/* Cloud-Native Nature */}
<ExploreMainProductCards className="max-lg:mx-4 max-lg:my-8" />
Expand Down
29 changes: 29 additions & 0 deletions packages/web/docs/src/components/error-boundary.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use client';

import { Component } from 'react';

export class ErrorBoundary extends Component<{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a bit absurd that this ins't exported from React, but I guess people have different error tracking needs and not everybody just plugs console.error into some useful log sink.

fallback: React.ReactNode;
children: React.ReactNode;
}> {
state = { hasError: false };

static getDerivedStateFromError(error: Error) {
console.error(error);
// Update state so the next render will show the fallback UI.
hasparus marked this conversation as resolved.
Show resolved Hide resolved
return { hasError: true };
}

componentDidCatch(error: Error, info: { componentStack: string }) {
console.error(error, info);
}

render() {
if (this.state.hasError) {
// You can render any custom fallback UI
hasparus marked this conversation as resolved.
Show resolved Hide resolved
return this.props.fallback;
}

return this.props.children;
}
}
Loading