DEVPROD-1435: Use renderingType GQL field to determine Row rendering format #486

SupaJoon · 2024-02-12T21:54:49Z

Description

The resmoke route is not fully deprecated yet so we must support it until Julian lets us know. renderingType is only applicable to test logs at the moment.

…lation

cypress · 2024-02-12T22:05:40Z

1 failed test on run #5222 ↗︎

1	120	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

fix it up
Project: Parsley	Commit: `30e548e0b8`
Status: Failed	Duration: 03:15 💡
Started: Feb 29, 2024 9:38 PM	Ended: Feb 29, 2024 9:41 PM

Review all test suite changes for PR #486 ↗︎

khelif96

I think this can be done in a way that is a little more flexible for expanding to different rendering types. Right now the assumption is that there are only 2 types of logs and doesn't leave room for future log types like how the previous implementation did.

If we are going to be using renderingType to determine log render behavior we should try to be consistent across the app about how it is used instead of having it only apply to a very specific subset of test logs.

khelif96 · 2024-02-14T17:26:12Z

src/components/LogRow/RowRenderer/index.tsx

+    // At this point, logMetadata.renderingType is guaranteed to be defined from LogView/LoadingPage.tsx.
+    // logType can be removed from this calculation when the Parsley resmoke route is fully deprecated.
+    const Row =
+      logMetadata?.renderingType === "resmoke" ||


Can we use enums instead of strings like "resmoke" or "default"

I also don't quite understand this bit of logic. Is there a way we can preserve rowRenderMap it seems far more flexible and would allow the expansion of future row types in the future.

I think this logic doesn't make sense because it doesn't do a good job at holistically bringing in renderingType to Parsley. The logic that you reviewed utilizes parsley route (logType) and renderingType when available. I just pushed some code to decouple the route from renderingType, utilizes testlog renderingType from the GQL endpoint but also extrapolates rendering type per logType.

khelif96 · 2024-02-14T17:27:38Z

src/context/LogContext/types.ts

@@ -9,18 +9,19 @@ interface SearchState {
 }

 interface LogMetadata {


Thanks for sorting!

khelif96 · 2024-02-14T19:28:44Z

src/pages/LogDrop/FileDropper.tsx

+          isUploadedLog: true,
+          logType,
+          renderingType:
+            logType === LogTypes.RESMOKE_LOGS ? "resmoke" : "default",


Same comment about using an enum value.

khelif96 · 2024-02-14T19:29:56Z

src/pages/LogDrop/ParseLogSelect/index.tsx

+type SelectState =
+  | LogTypes.RESMOKE_LOGS
+  | LogTypes.EVERGREEN_TASK_LOGS
+  | undefined;
+
 const ParseLogSelect: React.FC<ParseLogSelectProps> = ({
  fileName,
  onCancel,
  onParse,
 }) => {
-  const [logType, setLogType] = useState<LogTypes | undefined>(
-    (Cookie.get(LAST_SELECTED_LOG_TYPE) as LogTypes) ?? undefined,
+  const [logType, setLogType] = useState<SelectState>(
+    (Cookie.get(LAST_SELECTED_LOG_TYPE) as SelectState) ?? undefined,


Why did you make this change it seems to do the same thing but is arguably slightly less readable.

LogTypes includes values that are unrelated to the Select component state which makes it harder to infer usage. We should be more strict with our typings and exclude unused values when representing state.

khelif96 · 2024-02-14T19:37:06Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

+
+  // renderingType is only propagated for evergreen test logs as of DEVPROD-1435.
+  const renderingType =
+    (testData?.task?.tests.testResults[0]?.logs


renderingType will default to default for resmoke logs which is confusing. I think we should try to be consistent about how this value is treated and used.

khelif96 · 2024-02-16T17:03:43Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

@@ -108,6 +111,7 @@ export const useResolveLogURL = ({
  let htmlLogURL = "";
  let jobLogsURL = "";
  let legacyJobLogsURL = "";
+  let renderingType = "";


Suggested change

let renderingType = "";

let renderingType: SupportedLogRenderingTypes = LogRenderingTypes.Default;

This is still necessary. Otherwise the type alias on line 206 has no effect.

khelif96 · 2024-02-16T17:06:36Z

src/components/LogRow/RowRenderer/index.tsx

+      reportError(new Error("Encountered unsupported renderType"), {
+        rawLogURL: logMetadata?.rawLogURL,
+        renderingType: logMetadata?.renderingType,
+      }).warning();


We wouldn't want to report an error here. This will result in us logging an error for every single row.

I agree it's better to report the error closer to ingestion. Technically this wouldn't run into an error on every line because this code is in the generator body and not the return value.

khelif96 · 2024-02-16T17:07:08Z

src/components/LogRow/RowRenderer/index.tsx

@@ -38,6 +40,24 @@ const ParsleyRow: RowRendererFunction = ({ logType, processedLogLines }) => {
      ? new RegExp(`${highlights.map((h) => `(${h})`).join("|")}`, "gi")
      : undefined;

+  let Row: React.FC<ResmokeRowProps> | React.FC<AnsiRowProps>;
+  // At this point, logMetadata is defined from <LoadingPage />


I think we can remove this comment since its implied.

I will find this comment helpful in the future. If I were looking at this code after time away, this is something I would want to verify because logMetadata and renderingType are both marked as optional in their type structures.

khelif96 · 2024-02-21T23:44:19Z

src/constants/enums.ts

@@ -5,6 +5,11 @@ enum LogTypes {
  RESMOKE_LOGS = "RESMOKE_LOGS",
 }

+enum SupportedLogRenderingTypes {


Can we remove the word Supported since it being here implies that it is supported?

Suggested change

enum SupportedLogRenderingTypes {

enum LogRenderingTypes {

Originally, I wanted to make a distinction between unsupported and supported rendering types because unsupported types may be cached in the Apollo request layer and the code may encounter unsupported types. Either way works and I updated the code to your suggestion.

khelif96 · 2024-02-21T23:51:12Z

src/components/LogRow/RowRenderer/index.tsx

+      reportError(new Error("Encountered unsupported renderingType"), {
+        rawLogURL: logMetadata?.rawLogURL,
+        renderingType: logMetadata?.renderingType,
+      }).warning();


We shouldn't log this here since it would log for every single log row we try to render.

khelif96 · 2024-02-21T23:55:54Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

@@ -189,17 +200,21 @@ export const useResolveLogURL = ({
          text: false,
        });
      downloadURL = rawLogURL;
+      renderingType =
+        renderingTypeFromQuery || SupportedLogRenderingTypes.Default;
      break;
    }
    default:


This is probably a good place to error. We should set also probably set a default rendering type so the page can still work.

khelif96 · 2024-02-21T23:59:02Z

src/pages/LogDrop/ParseLogSelect/index.tsx

+type SelectState =
+  | LogTypes.RESMOKE_LOGS
+  | LogTypes.EVERGREEN_TASK_LOGS
+  | undefined;


I think maybe the select states should correspond to rendering types instead of LogTypes since with this refactor the LogTypes only really reflect the origin of the log.

khelif96 · 2024-02-22T00:00:22Z

src/pages/LogDrop/FileDropper.tsx

+          renderingType:
+            logType === LogTypes.RESMOKE_LOGS
+              ? SupportedLogRenderingTypes.Resmoke
+              : SupportedLogRenderingTypes.Default,


Doing the above we can also avoid this.

If I change the function parameter from LogTypes to LogRenderingType, then I will have to cast the function parameter to LogType for the logType value on line 47 which is fine for now. These tickets will look more into logType usage.
https://jira.mongodb.org/browse/DEVPROD-5130
https://jira.mongodb.org/browse/DEVPROD-5163

hmm i think this may be ok, Since this could be used to distinguish between log types for log sectioning project? I do think it would be important to iron that out in your project or at some point before it.

khelif96 · 2024-02-22T00:01:36Z

src/components/LogRow/RowRenderer/index.tsx

@@ -38,6 +40,24 @@ const ParsleyRow: RowRendererFunction = ({ logType, processedLogLines }) => {
      ? new RegExp(`${highlights.map((h) => `(${h})`).join("|")}`, "gi")
      : undefined;

+  let Row: React.FC<ResmokeRowProps> | React.FC<AnsiRowProps>;


This is more flexible and avoids exporting the the Row props.

Suggested change

let Row: React.FC<ResmokeRowProps> | React.FC<AnsiRowProps>;

let Row: typeof ResmokeRow | typeof AnsiRow;

khelif96

nit. Can you capitalize DEVPROD in the pr title

Great job just a few more comments and follow up from my last review.

khelif96 · 2024-02-28T19:30:36Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

+      const {
+        renderingType: renderingTypeFromQuery,
+        url,
+        urlRaw,
+      } = testData?.task?.tests.testResults[0]?.logs || {};


Can we leave a comment here.
Something simple such as if we fetched test results use that URL otherwise fallback to hardcoded routes?

I left a comment a comment for the query at the top of the file that describes how LogRenderingType is reasoned. I'm not actually sure what to comment here that would be useful for me. Please suggest a comment and I will commit it.

khelif96 · 2024-02-28T19:31:40Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

@@ -108,6 +111,7 @@ export const useResolveLogURL = ({
  let htmlLogURL = "";
  let jobLogsURL = "";
  let legacyJobLogsURL = "";
+  let renderingType = "";


This is still necessary. Otherwise the type alias on line 206 has no effect.

khelif96 · 2024-02-28T19:38:11Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

+      } else if (!renderingTypeFromQuery) {
+        renderingType = LogRenderingTypes.Default;
+      } else {
+        renderingType = LogRenderingTypes.Default;


I think this can be refactored out. The first if statement handles the case where renderingTypeFromQuery is undefined. renderingTypeFromQuery can't be in LogRenderingTypes

If we assign a default value for renderingType like my suggestion on on L115 this assignment in the else statement becomes unnecessary as well

the value from the query will overwrite renderingType so I still need the condition on line 206

I may be missing what you mean but Afaict renderingType doesn't get overridden anywhere else so I still think it's not necessary. Can you point me to where it does?

Ah, you're right. I jumbled the variable assignment on line 187 when I made that comment.

Ah, so renderingTypeFromQuery may actually be an empty string. In that case, I would want to cast it to the default rendering type and not submit an error -- that's when the condition on 206 comes into play.

khelif96 · 2024-02-28T19:39:56Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

+            rawLogURL,
+            unsupportedRenderingType: renderingTypeFromQuery,
+          },
+        }).warning();


This should probably be logged at .severe() since this indicates we encountered some sort of unsupported rendering type.

khelif96

Great job on this! 🚀 mod my comment

khelif96 · 2024-02-29T16:19:21Z

src/pages/LogView/LoadingPage/useResolveLogURLAndRenderingType.ts

+      } else if (!renderingTypeFromQuery) {
+        renderingType = LogRenderingTypes.Default;
+      } else {
+        renderingType = LogRenderingTypes.Default;


I may be missing what you mean but Afaict renderingType doesn't get overridden anywhere else so I still think it's not necessary. Can you point me to where it does?

SupaJoon added 6 commits February 12, 2024 14:49

rename export

7bb9408

rename files

5045396

rename gql file

9ea9e96

more gql query refactoring

72a1747

save renderingType to context and utilize in LogRow/RowRenderer calcu…

b24f4be

…lation

refactor

3ba2d91

SupaJoon requested a review from a team February 12, 2024 21:54

SupaJoon changed the title ~~DevProd-1435: Use renderType GQL field to determine Row rendering format~~ DevProd-1435: Use renderingType GQL field to determine Row rendering format Feb 13, 2024

khelif96 suggested changes Feb 14, 2024

View reviewed changes

SupaJoon added 3 commits February 15, 2024 14:06

improve data flow

d058bd7

use enum

bd6c31c

codegen

07a9829

SupaJoon requested a review from khelif96 February 15, 2024 20:03

SupaJoon added 2 commits February 21, 2024 09:48

update error message

aab1d89

Merge branch 'main' of github.com:evergreen-ci/parsley into DEVPROD-1435

9adf6ef

khelif96 suggested changes Feb 22, 2024

View reviewed changes

SupaJoon added 6 commits February 26, 2024 13:08

Merge branch 'main' of github.com:evergreen-ci/parsley into DEVPROD-1435

56a66b4

rename variable and refactor report error usage

abd93b9

update select state

191e8d0

refactor types

7c8be18

update comment

427599a

update options

baee031

SupaJoon requested a review from khelif96 February 27, 2024 15:44

fix test

56eae27

khelif96 reviewed Feb 28, 2024

View reviewed changes

SupaJoon changed the title ~~DevProd-1435: Use renderingType GQL field to determine Row rendering format~~ DEVPROD-1435: Use renderingType GQL field to determine Row rendering format Feb 29, 2024

SupaJoon added 2 commits February 29, 2024 10:22

comment

c8d0632

CR

38522e8

SupaJoon requested a review from khelif96 February 29, 2024 15:50

khelif96 approved these changes Feb 29, 2024

View reviewed changes

SupaJoon added 2 commits February 29, 2024 16:20

fix test

c976ed5

fix it up

30e548e

SupaJoon mentioned this pull request Mar 1, 2024

DEVPROD-1435: Use renderingType GQL field to determine Row rendering format #497

Merged

SupaJoon closed this Mar 1, 2024

		@@ -9,18 +9,19 @@ interface SearchState {
		}

		interface LogMetadata {

	let renderingType = "";
	let renderingType: SupportedLogRenderingTypes = LogRenderingTypes.Default;

	let Row: React.FC<ResmokeRowProps> \| React.FC<AnsiRowProps>;
	let Row: typeof ResmokeRow \| typeof AnsiRow;

DEVPROD-1435: Use renderingType GQL field to determine Row rendering format #486

DEVPROD-1435: Use renderingType GQL field to determine Row rendering format #486

Conversation

SupaJoon commented Feb 12, 2024

Description

cypress bot commented Feb 12, 2024 • edited Loading

1 failed test on run #5222 ↗︎

Review all test suite changes for PR #486 ↗︎

khelif96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khelif96 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khelif96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khelif96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Feb 12, 2024 •

edited

Loading

SupaJoon Feb 15, 2024 •

edited

Loading

SupaJoon Feb 14, 2024 •

edited

Loading

khelif96 Feb 16, 2024 •

edited

Loading

SupaJoon Feb 27, 2024 •

edited

Loading

SupaJoon Feb 26, 2024 •

edited

Loading

SupaJoon Feb 29, 2024 •

edited

Loading

SupaJoon Feb 29, 2024 •

edited

Loading