Bug 1995932 - Add detailed log parsing status display in job detail panel #9031
Conversation
junngo
force-pushed
the
display-job-log-status
branch
from
71d9917 to
7f28a13
Compare
There was a problem hiding this comment.
Thanks for the quick fix @junngo! We'll also need to handle this here:
We should double-check to see if there's any other code that is checking if all the job logs are parsed and exclude the perfherder artifacts from them.
junngo
force-pushed
the
display-job-log-status
branch
2 times, most recently
from
8069226 to
e1a86ac
Compare
There was a problem hiding this comment.
Looks much better now, thanks! I noticed that @Archaeopteryx landed a patch here that prevents a perfherder-data artifact from being interpreted as a log: e231e99
It looks like it's only targetting a specific artifact though so it would be good to modify that to catch any perfherder-data artifact.
junngo
force-pushed
the
display-job-log-status
branch
from
e1a86ac to
4e467f4
Compare
| {/* Raw Log */} | ||
| <LogItem | ||
| logUrls={logUrlsUseful} | ||
| logUrls={logUrls} |
There was a problem hiding this comment.
Hi @Archaeopteryx, Sorry for the inconvenience. I've reverted the previous change. The filtered list is now injected via the code below [0][1]
If this doesn’t meet our standards, please feel free to leave a comment and I’ll update accordingly.
Thanks :)
[0]
ui/job-view/details/summary/SummaryPanel.jsx
[1]
https://github.com/mozilla/treeherder/pull/9031/files#diff-fa5ec09ffccd4ca6ea0a2554ffb352281db4896b75380d66deaf105c6f332bb8R71
There was a problem hiding this comment.
@junngo I think we still want to keep these changes since I don't see any modifications to where this LogUrls method is being used. We just need to make it apply to all perfherder-data artifacts.
See here for where this is being used:
There was a problem hiding this comment.
Hi @junngo . The perfherder artifact had to be ignored by me because with more than one artifact treated as log, the buttons to open the parsed or plain text log(s) turn from buttons into button with menus and require more clicks for the desired action. Investigation of issues with the these logs is a common task of code sheriffs.
There was a problem hiding this comment.
Thanks, got it! I’ll make sure to keep ignoring the perfherder artifacts in this menu going forward :)
junngo
force-pushed
the
display-job-log-status
branch
from
4e467f4 to
fb9a38b
Compare
| const { logUrls, logViewerUrl, logViewerFullUrl } = props; | ||
| const logUrlsUseful = logUrls.filter( | ||
| (logUrl) => logUrl.name !== 'perfherder-data-resource-usage.json', | ||
| (logUrl) => !logUrl.name.startsWith('perfherder-data'), |
There was a problem hiding this comment.
@gmierz Thanks for pointing that out :)
I updated the filtering logic to exclude all perfherder-data* artifacts instead of only perfherder-data-resource-usage.json.
There was a problem hiding this comment.
Ah sorry Jun, but I just noticed that we'll want to change this, and other checks to use includes instead of startswith so that we're consistent with how we pick up these artifacts here:
treeherder/treeherder/etl/job_loader.py
Line 192 in e231e99
There was a problem hiding this comment.
No problem :)
Would it make sense to use includes here[0][1] as well for consistency?
[0]
https://github.com/mozilla/treeherder/pull/9031/files#diff-fa5ec09ffccd4ca6ea0a2554ffb352281db4896b75380d66deaf105c6f332bb8R30
[1]
https://github.com/mozilla/treeherder/pull/9031/files#diff-fa5ec09ffccd4ca6ea0a2554ffb352281db4896b75380d66deaf105c6f332bb8R33
There was a problem hiding this comment.
Thanks, sparky :) I updated using includes for consistency.
junngo
force-pushed
the
display-job-log-status
branch
from
fb9a38b to
eff4f92
Compare
junngo
force-pushed
the
display-job-log-status
branch
from
eff4f92 to
923c4dc
Compare
3 participants
I added individual job log entries under the “Log parsing status” section to make each item easier to distinguish.
before:
after:
bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1995932