DEVPROD-2643: Fix useLogDownloader bug #436

sophstad · 2023-11-15T15:53:18Z

DEVPROD-2643

Description

First commit reverts Revert "DEVPROD-929: Use task log links from API" #434 i. e. reintroduces DEVPROD-929: Use task log links from API #431
Second commit fixes bug
- Remove useLogDownloader dependency on isEvergreenLoading. This dependency caused the hook to run multiple times; when the bug was deployed to production you could observe multiple requests to Logkeeper. I think the subsequent request to Logkeeper also resulted in the first one being terminated, hence the "Incomplete log" warnings we saw earlier today.
- In the case of an invalid task URL (i.e. /evergreen/invalid/0/task) where the GraphQL query doesn't return a download link, use the previous approach to construct a link given the task ID, execution, etc.

Testing

Verified on beta that removing this dependency results in a single call to fetch the log. It's deployed to beta right now if you care to look!

This reverts commit b0bd2e1.

cypress · 2023-11-15T16:07:47Z

1 failed test on run #4226 ↗︎

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

Details:

Clean up
Project: Parsley	Commit: `dbf8182f82`
Status: Failed	Duration: 03:49 💡
Started: Nov 28, 2023 10:11 PM	Ended: Nov 28, 2023 10:15 PM

Review all test suite changes for PR #436 ↗︎

minnakt

sorry for the late review 😭 pls ping me directly whenever you want a re-review!

minnakt · 2023-11-17T03:37:58Z

src/pages/LogView/LoadingPage/useResolveLogURL.test.tsx

    await waitForNextUpdate();
+    await waitForNextUpdate();


the double await here strikes me as odd... could it be removed? 🤔

My mistake thank you!

minnakt · 2023-11-17T03:40:56Z

src/pages/LogView/LoadingPage/useResolveLogURL.test.tsx

not sure if this is a problem on my end, but when I run this test suite a few times locally I see quite a bit of flakiness. I also tried switching to the main branch and running the test suite a bunch of times, but I didn't see any flakiness there

minnakt · 2023-11-17T04:01:56Z

src/constants/logURLTemplates.ts

-  options: { priority?: boolean; text?: boolean }
+  logLinks: TaskType["logs"],
+  origin: string,
+  params: { [key: string]: any } = {}


does it make sense to type the params a bit more strongly?

This seemed to be the general typing we use for query params, à la useQueryParams

parsley/src/hooks/useQueryParam/index.ts

Line 17 in 4126a39

(params: { [key: string]: any }) => {

I thought it might be nice to type with options: { priority?: boolean; text?: boolean } since we know what params to expect and constructEvergreenTaskLogURL has that type definition!

minnakt · 2023-11-17T04:11:17Z

src/hooks/useLogDownloader/index.ts

@@ -17,28 +17,35 @@ import {
 import { fetchLogFile } from "utils/fetchLogFile";


it's not a big deal, but do you know where this flash of 0 comes from? (only happens on bigger logs)

Screen.Recording.2023-11-16.at.11.09.32.PM.mov

I actually think this is a React strict-mode quirk that occurs locally and results in certain things firing twice! We've been running into more of those lately. I can reproduce this on main when running locally, and don't see the behavior when my branch is deployed to beta.

minnakt · 2023-11-17T04:13:27Z

src/hooks/useLogDownloader/index.ts

@@ -142,6 +150,8 @@ const useLogDownloader = (
          });
          setIsLoading(false);
        });
+    } else if (url === null) {
+      setError("Log URL not specified, unable to download.");


I think i might tweak the wording here to something along the lines of this:

Suggested change

setError("Log URL not specified, unable to download.");

setError("Could not retrieve log URL from task, unable to download.");

…evert-434-revert-431-DEVPROD-929

minnakt

lgtm!

minnakt · 2023-11-28T19:35:48Z

src/constants/logURLTemplates.ts

-  options: { priority?: boolean; text?: boolean }
+  logLinks: TaskType["logs"],
+  origin: string,
+  params: { [key: string]: any } = {}


I thought it might be nice to type with options: { priority?: boolean; text?: boolean } since we know what params to expect and constructEvergreenTaskLogURL has that type definition!

minnakt · 2023-11-28T19:41:30Z

src/pages/LogView/LoadingPage/useResolveLogURL.ts

+        : constructEvergreenTaskLogURL(taskID, execution, origin as any, {
+            priority: true,
+            text: true,
+          });
+      rawLogURL = task?.logs
+        ? getEvergreenTaskLogURL(task.logs, origin, {
+            text: true,
+          })
+        : constructEvergreenTaskLogURL(taskID, execution, origin as any, {
+            text: true,
+          });
+      htmlLogURL = task?.logs
+        ? getEvergreenTaskLogURL(task.logs, origin, {
+            text: false,
+          })
+        : constructEvergreenTaskLogURL(taskID, execution, origin as any, {


wondering if it makes sense to copy the types for getEvergreenTaskLogURL to remove the origin as any stuff!

minnakt · 2023-11-28T19:41:57Z

src/pages/LogView/LoadingPage/useResolveLogURL.test.tsx

@@ -184,7 +254,7 @@ const getEmptyTestLogURLMock: ApolloMock<
    variables: {
      execution: 0,
      taskID: "a-task-id",
-      testName: "^a-test-name$",
+      testName: "^a-test-name-that-doesnt-exist$",


ty for fixing 😁

Revert "Revert "DEVPROD-929: Use task log links from API" (#434)"

595d9d0

This reverts commit b0bd2e1.

sophstad mentioned this pull request Nov 15, 2023

DEVPROD-2643: Remove useLogDownloader hook dependency #435

Closed

Fix useLogDownloader hook bug

3ddc084

sophstad changed the title ~~Revert "Revert "DEVPROD-929: Use task log links from API""~~ DEVPROD-2643: Fix useLogDownloader bug Nov 15, 2023

sophstad requested a review from a team November 15, 2023 16:19

sophstad marked this pull request as ready for review November 15, 2023 16:19

minnakt reviewed Nov 17, 2023

View reviewed changes

sophstad added 4 commits November 27, 2023 16:03

Merge branch 'main' of https://github.com/evergreen-ci/parsley into r…

25cc00b

…evert-434-revert-431-DEVPROD-929

Fixes

ad35cf0

Use fallback task log URL

27042f4

Codegen

2b3d6f2

sophstad requested a review from minnakt November 28, 2023 18:15

minnakt approved these changes Nov 28, 2023

View reviewed changes

Clean up

dbf8182

sophstad merged commit 1e71b12 into main Nov 28, 2023
2 checks passed

sophstad deleted the revert-434-revert-431-DEVPROD-929 branch November 28, 2023 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEVPROD-2643: Fix useLogDownloader bug #436

DEVPROD-2643: Fix useLogDownloader bug #436

sophstad commented Nov 15, 2023 •

edited

Loading

cypress bot commented Nov 15, 2023 •

edited

Loading

minnakt left a comment

minnakt Nov 17, 2023

sophstad Nov 28, 2023

minnakt Nov 17, 2023

minnakt Nov 17, 2023

sophstad Nov 27, 2023

minnakt Nov 28, 2023

minnakt Nov 17, 2023

sophstad Nov 27, 2023

minnakt Nov 17, 2023

minnakt left a comment

minnakt Nov 28, 2023

minnakt Nov 28, 2023

minnakt Nov 28, 2023

		@@ -17,28 +17,35 @@ import {
		import { fetchLogFile } from "utils/fetchLogFile";

	setError("Log URL not specified, unable to download.");
	setError("Could not retrieve log URL from task, unable to download.");

DEVPROD-2643: Fix useLogDownloader bug #436

DEVPROD-2643: Fix useLogDownloader bug #436

Conversation

sophstad commented Nov 15, 2023 • edited Loading

Description

Testing

cypress bot commented Nov 15, 2023 • edited Loading

1 failed test on run #4226 ↗︎

Review all test suite changes for PR #436 ↗︎

minnakt 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

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

minnakt 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

sophstad commented Nov 15, 2023 •

edited

Loading

cypress bot commented Nov 15, 2023 •

edited

Loading