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

Improve log display #3768

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlanGreene
Copy link
Member

@AlanGreene AlanGreene commented Nov 22, 2024

Changes

Related to #2306

TODO:

  • resolve remaining design questions:
    • allow unselecting all log levels?
    • how to improve discoverability of filter settings? Some indication that lines are hidden, including number?
  • add documentation describing the new functionality and supported log format
  • release note

Add support for toggling display of timestamps in the logs, so the user no longer has to navigate away to the settings page to do this. This is available on both PipelineRun and TaskRun pages. We always request timestamps and just show / hide them depending on the user's preference. This is persisted to browser localStorage as with the toggle on the settings page.

Add support for detecting GitHub Actions workflow command-style log levels in log output. This provides an improved user experience as it allows for filtering logs to hide unwanted noise, e.g. debug logs, by default. We may consider allowing the format to be customised in a future release depending on user feedback.

Refactor the styles so LogFormat now correctly owns most of the styling of the log content, with Log only responsible for additional styling of the container.

Refactor use of the LogsToolbar component to allow for customisable use by third-party consumers of the Dashboard components. This means they can much more easily take advantage of the new features, such as toggling timestamps and log levels, without having to reimplement the menu and related code themselves.

Eliminate redundant use of split and join calls when processing the logs, improving performance. LogFormat now receives an array of log line objects, pre-parsed into the new structure with the timestamp, level (optional), and message fields. Where a multiline log is encountered, the timestamp of the first line is reused for subsequent lines in that log.

Fix issue where in some cases a blank line did not reserve vertical space, leading to cramped display of logs. Now each line is guaranteed to occupy a minimum height, ensuring blank lines output in the logs to aid in readability are preserved in the UI.

Update FormattedDate to add support for displaying seconds, as this is quite important in the log context. Default to false for this setting so existing date / timestamps in other parts of the UI are unaffected. The full raw timestamp as received in the logs in displayed in a tooltip on hover.

Update unit tests to reflect the new and changed components and behaviours.

Update common PipelineRun E2E to exercise the new log toolbar and validate the log content is rendered as expected.

Add new stories to cover the new functionality. Update existing stories to demonstrate use of the new functionality in context.

Update Carbon:

  • resolve issue with Plex Mono font

    Some glyphs weren't included in the Plex version packaged with previous Carbon releases, resulting in broken formatting for some log content, e.g. using box characters to print tables.

    In @carbon/react 1.71.0 the Plex version has been updated, as well as changing how it's consumed. Instead of a single package with all of the font variants, they're now published as separate packages per font family. Add the $use-per-family-plex flag to our config to use these new packages. The custom $font-path is still required for compatibility with Vite.

  • resolve issue with duplicate onChange events from MenuItemSelectable

  • resolve issue with duplicate onChange events when clearing a ComboBox

Notes:

  • colours of the log level badges are based on the colours of the Carbon Tag component, with their opacity reduced so they're not as intense due to the potentially large number of them that could be displayed in the logs. These all meet minimum colour contrast ratio required for WCAG 2.0 level AA (i.e. > 4.5:1).
  • the default log level is 'info' if no log level is explicitly provided in the logs, however we only display the badge when the log level is explicitly set. This avoids unnecessary and unwanted noise / clutter in the logs when not using the new log format.
  • highlight and hover state included to highlight log lines, aiding in consuming the content, especially with longer log lines where the log level badge may not be adjacent to the content being read. A future update to the log viewer will add support for line wrapping but this is out of scope for this particular change.

/kind feature
/kind design
/kind bug
/kind misc
/kind documentation

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (new features, significant UI changes, API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

Add support for toggling display of timestamps in the logs, so
the user no longer has to navigate away to the settings page
to do this. This is available on both PipelineRun and TaskRun
pages. We always request timestamps and just show / hide them
depending on the user's preference. This is persisted to browser
localStorage as with the toggle on the settings page.

Add support for detecting GitHub Actions workflow command-style
log levels in log output. This provides an improved user
experience as it allows for filtering logs to hide unwanted
noise, e.g. debug logs, by default. We may consider allowing
the format to be customised in a future release depending on
user feedback.

Refactor the styles so `LogFormat` now correctly owns most
of the styling of the log content, with `Log` only responsible
for additional styling of the container.

Refactor use of the `LogsToolbar` component to allow for
customisable use by third-party consumers of the Dashboard
components. This means they can much more easily take advantage
of the new features, such as toggling timestamps and log levels,
without having to reimplement the menu and related code themselves.

Eliminate redundant use of `split` and `join` calls when processing
the logs, improving performance. `LogFormat` now receives an
array of log line objects, pre-parsed into the new structure
with the `timestamp`, `level` (optional), and `message` fields.
Where a multiline log is encountered, the timestamp of the first
line is reused for subsequent lines in that log.

Fix issue where in some cases a blank line did not reserve vertical
space, leading to cramped display of logs. Now each line is
guaranteed to occupy a minimum height, ensuring blank lines
output in the logs to aid in readability are preserved in the UI.

Update `FormattedDate` to add support for displaying seconds, as
this is quite important in the log context. Default to `false` for
this setting so existing date / timestamps in other parts of the
UI are unaffected. The full raw timestamp as received in the logs
in displayed in a tooltip on hover.

Update unit tests to reflect the new and changed components and
behaviours.

Update common PipelineRun E2E to exercise the new log toolbar
and validate the log content is rendered as expected.

Add new stories to cover the new functionality. Update existing
stories to demonstrate use of the new functionality in context.

Update Carbon:
- resolve issue with Plex Mono font

  Some glyphs weren't included in the Plex version packaged with
  previous Carbon releases, resulting in broken formatting for some
  log content, e.g. using box characters to print tables.

  In `@carbon/react` 1.71.0 the Plex version has been updated, as well
  as changing how it's consumed. Instead of a single package with all
  of the font variants, they're now published as separate packages per
  font family. Add the `$use-per-family-plex` flag to our config to
  use these new packages. The custom `$font-path` is still required
  for compatibility with Vite.

- resolve issue with duplicate onChange events from MenuItemSelectable

- resolve issue with duplicate onChange events when clearing a ComboBox

Notes:
- colours of the log level badges are based on the colours of the
  Carbon `Tag` component, with their opacity reduced so they're
  not as intense due to the potentially large number of them that
  could be displayed in the logs. These all meet minimum colour
  contrast ratio required for WCAG 2.0 level AA (i.e. > 4.5:1).
- the default log level is 'info' if no log level is explicitly
  provided in the logs, however we only display the badge when
  the log level is explicitly set. This avoids unnecessary and
  unwanted noise / clutter in the logs when not using the new
  log format.
- highlight and hover state included to highlight log lines, aiding
  in consuming the content, especially with longer log lines where
  the log level badge may not be adjacent to the content being read.
  A future update to the log viewer will add support for line
  wrapping but this is out of scope for this particular change.
@tekton-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/design Categorizes issue or PR as related to design. kind/bug Categorizes issue or PR as related to a bug. kind/misc Categorizes issue or PR as a miscellaneuous one. kind/documentation Categorizes issue or PR as related to documentation. labels Nov 22, 2024
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from alangreene after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@AlanGreene AlanGreene requested review from LyndseyBu and removed request for skaegi November 22, 2024 16:26
@AlanGreene
Copy link
Member Author

/test all

@AlanGreene
Copy link
Member Author

E2E failed on import resources test when selecting the import namespace. Not seeing the same issue locally, rerunning to see if it persists on prow

/test pull-tekton-dashboard-integration-tests

@AlanGreene
Copy link
Member Author

AlanGreene commented Nov 22, 2024

Failed in the same place. It's passing consistently locally, both in Electron and a real browser. Will need to debug next week…

@AlanGreene
Copy link
Member Author

/test pull-tekton-dashboard-integration-tests

@tekton-robot
Copy link
Contributor

@AlanGreene: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-tekton-dashboard-integration-tests 4746a50 link true /test pull-tekton-dashboard-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@briangleeson briangleeson left a comment

Choose a reason for hiding this comment

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

A few queries but overall looking good

src/containers/TaskRun/TaskRun.jsx Show resolved Hide resolved
src/containers/PipelineRun/PipelineRun.jsx Show resolved Hide resolved
src/containers/PipelineRun/PipelineRun.jsx Show resolved Hide resolved
export function getLogLevels() {
let logLevels = localStorage.getItem('tkn-logs-levels');
if (logLevels) {
logLevels = JSON.parse(logLevels);
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth wrapping in a try-catch and return the default logLevels in case an error is encountered parsing the JSON, if bad data somehow exists in tkn-logs-levels?

src/containers/LogsToolbar/LogsToolbar.jsx Show resolved Hide resolved
@briangleeson
Copy link
Contributor

One thing I noticed at https://alangreene.github.io/dashboard/?path=/story/components-log--toolbar, the dropdown menu disappears every time you change a logLevel option. Would be handy to be keep it up if user wants to add/remove multiple levels

@AlanGreene
Copy link
Member Author

AlanGreene commented Nov 26, 2024

dropdown menu disappears every time you change a logLevel option. Would be handy to be keep it up if user wants to add/remove multiple levels

Yes, that's something we've already discussed with design. Other similar tools behave in this way, including the Google Chrome dev tools. We may revisit based on user feedback and provide a richer popover similar to the Mezmo log level selection or something else entirely.

The thinking behind leaving it the way it currently is is that users are unlikely to be changing the selected log levels very often, so the extra clicks aren't overly burdensome. If this proves untrue, we'll explore other approaches.

@AlanGreene
Copy link
Member Author

Forgot to add an update this morning. The Carbon issue causing the E2E failure is carbon-design-system/carbon#18145, which explains why it wasn't reproduced locally in dev mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. kind/design Categorizes issue or PR as related to design. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. kind/misc Categorizes issue or PR as a miscellaneuous one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants