ui: remove TraceErrorController

Also update CI output to make it more actionable

Change-Id: I2c79590f436b3432a44c3f40fc3decd4ed40d21e

test/ci/ui_tests.sh

@@ -38,12 +38,17 @@ set +e
 ui/run-integrationtests --out ${OUT_PATH} --no-build
 RES=$?
 
+set +x
+
 # Copy the output of screenshots diff testing.
 if [ -d ${OUT_PATH}/ui-test-artifacts ]; then
   cp -a ${OUT_PATH}/ui-test-artifacts /ci/artifacts/ui-test-artifacts
   echo "UI integration test report with screnshots:"
   echo "https://storage.googleapis.com/perfetto-ci-artifacts/$PERFETTO_TEST_JOB/ui-test-artifacts/index.html"
   echo ""
+  echo "To download locally the changed screenshots run:"
+  echo "tools/download_changed_screenshots.py $PERFETTO_TEST_JOB"
+  echo ""
   echo "Perfetto UI build for this CL"
   echo "https://storage.googleapis.com/perfetto-ci-artifacts/$PERFETTO_TEST_JOB/ui/index.html"
   exit $RES

test/data/ui-screenshots/independent_features.test.ts/trace-error-notification/error-icon.png.sha256

@@ -0,0 +1 @@
+d11a2d89b1d96ede01644accc98cee5ac6fadb15a13d40baa78977fd5c212670

ui/src/common/fake_trace_impl.ts

@@ -40,6 +40,7 @@ export function createFakeTraceImpl(args: FakeTraceImplArgs = {}) {
     traceTzOffset: Time.ZERO,
     cpus: [],
     gpuCount: 0,
+    importErrors: 0,
   };
   return AppImpl.instance.newTraceInstance(
     new FakeEngine(args.allowQueries ?? false),

ui/src/controller/trace_controller.ts

@@ -50,9 +50,8 @@ import {
   WasmEngineProxy,
 } from '../trace_processor/wasm_engine_proxy';
 import {showModal} from '../widgets/modal';
-import {Child, Children, Controller} from './controller';
+import {Controller} from './controller';
 import {LoadingManager} from './loading_manager';
-import {TraceErrorController} from './trace_error_controller';
 import {
   TraceBufferStream,
   TraceFileStream,
@@ -227,15 +226,7 @@ export class TraceController extends Controller<States> {
         break;
 
       case 'ready':
-        // At this point we are ready to serve queries and handle tracks.
-        const engine = assertExists(this.engine);
-        const childControllers: Children = [];
-
-        childControllers.push(
-          Child('traceError', TraceErrorController, {engine}),
-        );
-
-        return childControllers;
+        return [];
 
       default:
         throw new Error(`unknown state ${this.state}`);
@@ -1111,6 +1102,7 @@ async function getTraceTimeDetails(
     traceTzOffset,
     cpus: await getCpus(engine),
     gpuCount: await getNumberOfGpus(engine),
+    importErrors: await getTraceErrors(engine),
     source: engineCfg.source,
   };
 }
@@ -1147,6 +1139,12 @@ async function getNumberOfGpus(engine: Engine): Promise<number> {
   return result.firstRow({gpuCount: NUM}).gpuCount;
 }
 
+async function getTraceErrors(engine: Engine): Promise<number> {
+  const sql = `SELECT sum(value) as errs FROM stats WHERE severity != 'info'`;
+  const result = await engine.query(sql);
+  return result.firstRow({errs: NUM}).errs;
+}
+
 async function getTracingMetadataTimeBounds(engine: Engine): Promise<TimeSpan> {
   const queryRes = await engine.query(`select
        name,

ui/src/frontend/globals.ts

@@ -92,7 +92,6 @@ class Globals {
   private _numQueriesQueued = 0;
   private _bufferUsage?: number = undefined;
   private _recordingLog?: string = undefined;
-  private _traceErrors?: number = undefined;
   private _metricError?: string = undefined;
   private _jobStatus?: Map<ConversionJobName, ConversionJobStatus> = undefined;
   private _embeddedMode?: boolean = undefined;
@@ -102,7 +101,6 @@ class Globals {
   httpRpcState: HttpRpcState = {connected: false};
   showPanningHint = false;
   permalinkHash?: string;
-  showTraceErrorPopup = true;
   extraSqlPackages: SqlPackage[] = [];
 
   get workspace(): Workspace {
@@ -248,14 +246,6 @@ class Globals {
     return assertExists(this._threadMap);
   }
 
-  get traceErrors() {
-    return this._traceErrors;
-  }
-
-  setTraceErrors(arg: number) {
-    this._traceErrors = arg;
-  }
-
   get metricError() {
     return this._metricError;
   }

ui/src/frontend/publish.ts

@@ -77,11 +77,6 @@ export function publishRecordingLog(args: {logs: string}) {
   globals.publishRedraw();
 }
 
-export function publishTraceErrors(numErrors: number) {
-  globals.setTraceErrors(numErrors);
-  globals.publishRedraw();
-}
-
 export function publishMetricError(error: string) {
   globals.setMetricError(error);
   globals.publishRedraw();

ui/src/frontend/topbar.ts

@@ -21,6 +21,7 @@ import {Popup, PopupPosition} from '../widgets/popup';
 import {assertFalse} from '../base/logging';
 import {OmniboxMode} from '../core/omnibox_manager';
 import {AppImpl} from '../core/app_trace_impl';
+import {Trace, TraceAttrs} from '../public/trace';
 
 export const DISMISSED_PANNING_HINT_KEY = 'dismissedPanningHint';
 
@@ -75,12 +76,15 @@ class HelpPanningNotification implements m.ClassComponent {
   }
 }
 
-class TraceErrorIcon implements m.ClassComponent {
-  view() {
+class TraceErrorIcon implements m.ClassComponent<TraceAttrs> {
+  private tracePopupErrorDismissed = false;
+
+  view({attrs}: m.CVnode<TraceAttrs>) {
+    const trace = attrs.trace;
     if (globals.embeddedMode) return;
 
     const mode = AppImpl.instance.omnibox.mode;
-    const errors = globals.traceErrors;
+    const errors = trace.traceInfo.importErrors;
     if (
       (!Boolean(errors) && !globals.metricError) ||
       mode === OmniboxMode.Command
@@ -96,11 +100,11 @@ class TraceErrorIcon implements m.ClassComponent {
         Popup,
         {
           trigger: m('.popup-trigger'),
-          isOpen: globals.showTraceErrorPopup,
+          isOpen: !this.tracePopupErrorDismissed,
           position: PopupPosition.Left,
           onChange: (shouldOpen: boolean) => {
             assertFalse(shouldOpen);
-            globals.showTraceErrorPopup = false;
+            this.tracePopupErrorDismissed = true;
           },
         },
         m('.error-popup', 'Data-loss/import error. Click for more info.'),
@@ -122,6 +126,7 @@ class TraceErrorIcon implements m.ClassComponent {
 
 export interface TopbarAttrs {
   omnibox: m.Children;
+  trace?: Trace;
 }
 
 export class Topbar implements m.ClassComponent<TopbarAttrs> {
@@ -133,7 +138,7 @@ export class Topbar implements m.ClassComponent<TopbarAttrs> {
       omnibox,
       m(Progress),
       m(HelpPanningNotification),
-      m(TraceErrorIcon),
+      attrs.trace && m(TraceErrorIcon, {trace: attrs.trace}),
     );
   }
 }

ui/src/frontend/ui_main.ts

@@ -677,6 +677,7 @@ export class UiMainPerTrace implements m.ClassComponent {
         m(Sidebar),
         m(Topbar, {
           omnibox: this.renderOmnibox(),
+          trace: this.trace,
         }),
         m(Alerts),
         children,

ui/src/public/trace_info.ts

@@ -41,6 +41,9 @@ export interface TraceInfo {
 
   // The number of gpus in the trace
   readonly gpuCount: number;
+
+  // The number of import/analysis errors present in the `stats` table.
+  readonly importErrors: number;
 }
 
 export interface TraceFileSource {

ui/src/test/independent_features.test.ts

@@ -32,3 +32,12 @@ test('debuggable chip', async ({browser}) => {
   await pth.toggleTrackGroup(trackGroup);
   await pth.waitForIdleAndScreenshot('track_with_debuggable_chip_expanded.png');
 });
+
+test('trace error notification', async ({browser}) => {
+  const page = await browser.newPage();
+  const pth = new PerfettoTestHelper(page);
+  await pth.openTraceFile('clusterfuzz_14753');
+  await pth.waitForIdleAndScreenshot('error-icon.png', {
+    clip: {x: 1800, y: 0, width: 150, height: 150},
+  });
+});

ui/src/test/perfetto_ui_test_helper.ts

@@ -63,7 +63,6 @@ export class PerfettoTestHelper {
     assertExists(file).setInputFiles(tracePath);
     await this.waitForPerfettoIdle();
     await this.page.mouse.move(0, 0);
-    await this.page.mouse.click(0, 0);
   }
 
   waitForPerfettoIdle(idleHysteresisMs?: number): Promise<void> {