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.

Sign up for GitHub

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

Jump to bottom

fix(console): fix column sorting in resources tab (#488) #491

Merged
merged 2 commits into from
Nov 29, 2023
Merged

fix(console): fix column sorting in resources tab (#488) #491

merged 2 commits into from
Nov 29, 2023

Conversation

mbiggio
Copy link
Contributor

@mbiggio mbiggio commented Nov 22, 2023

In the Resources tab, columns names were being associates to the wrong sorting keys, and some column names were just missing from the SortBy implementation.

All columns have been associated to the corresponding sorting keys, and for the Location column, a structured type has been stored in the resource object, in order to sort lexicographically on the path and with conventional numeric ordering on row and column. Also, sorting for attributes as been implemented by sorting lexicographically on the first attribute of each resource row, even if that is probably sub-optimal

@mbiggio mbiggio requested a review from a team as a code owner November 22, 2023 01:44
hds
hds requested changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

Hi! 👋 Thank your PR!

I really like the change to fix the location sorting, that's a really great idea. However, I'd like to propose that we break that out into a separate PR. The reasons are:

  1. It will make this PR smaller and more focussed (we will still have location sorting, it will just be sub-optimal)
  2. I would propose that we additionally store the location as a string (actually InternedStr) on the task and resource location. This will avoid the need for extra allocations every time we redraw and I expect that it will be quite efficient as normally there are only a limited number of locations that tasks and resources get created from.

How does that sound?

@mbiggio
Copy link
Contributor Author

mbiggio commented Nov 22, 2023

Hi! 👋 Thank your PR!

I really like the change to fix the location sorting, that's a really great idea. However, I'd like to propose that we break that out into a separate PR. The reasons are:

  1. It will make this PR smaller and more focussed (we will still have location sorting, it will just be sub-optimal)
  2. I would propose that we additionally store the location as a string (actually InternedStr) on the task and resource location. This will avoid the need for extra allocations every time we redraw and I expect that it will be quite efficient as normally there are only a limited number of locations that tasks and resources get created from.

How does that sound?

Hi @hds, yeah that makes a lot of sense, thanks for the comment!
Will change this PR and open a new one for the location

hds
hds requested changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

One small nit regarding the imports. Thank you!

tokio-console/src/state/resources.rs Outdated Show resolved Hide resolved
hds
hds approved these changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@hds hds left a comment

Choose a reason for hiding this comment

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

Thank you! Approving now, and I'll merge later when I'm on my computer again (merging from the mobile app has some problems).

hawkw
hawkw reviewed Nov 22, 2023
View reviewed changes
tokio-console/src/state/resources.rs Outdated Show resolved Hide resolved
tokio-console/src/state/resources.rs Outdated Show resolved Hide resolved
@mbiggio mbiggio force-pushed the wip/biggio/sorting-resources branch 2 times, most recently from 6078f1c to fc72d2c Compare November 23, 2023 09:37
fix(console): fix column sorting in resources tab (tokio-rs#488)
0aa1bbe 
In the Resources tab, column names were being associated
to the wrong sorting keys, and some columns names were just
missing from the `SortBy` implementation.

Sorting for attributes, that was previously missing, has been
implemented by sorting lexicographically on the key of the first
attribute of each resource row, even if that is probably sub-optimal.
@hds hds merged commit 96c65bd into tokio-rs:main Nov 29, 2023
11 checks passed
@github-actions github-actions bot mentioned this pull request Jun 7, 2024
Merged
hds pushed a commit that referenced this pull request Jun 10, 2024
@github-actions
chore: release tokio-console-v0.1.11, console-api-v0.7.0, console-sub…
b802bf1 
…scriber-v0.3.0 (#557)

## 🤖 New release
* `tokio-console`: 0.1.10 -> 0.1.11
* `console-api`: 0.6.0 -> 0.7.0
* `console-subscriber`: 0.2.0 -> 0.3.0


## `tokio-console`

## tokio-console-v0.1.11 - (2024-06-10)

### Added

- Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa))
- Add flags and configurations for warnings ([#493](#493)) ([174883f](174883f))
- Add `--allow` flag ([#513](#513)) ([8da7037](8da7037))

### Documented

- Add note about running on Windows ([#510](#510)) ([a0d20fd](a0d20fd))

### Fixed

- Ignore key release events ([#468](#468)) ([715713a](715713a))
- Accept only `file://`, `http://`, `https://` URI ([#486](#486)) ([031bddd](031bddd))
- Fix column sorting in resources tab ([#491](#491)) ([96c65bd](96c65bd))
- Only trigger lints on async tasks ([#517](#517)) ([4593222](4593222))
- Remove duplicate controls from async ops view ([#519](#519)) ([f28ba4a](f28ba4a))
- Add pretty format for 'last woken' time ([#529](#529)) ([ea11ad8](ea11ad8))


## `console-api`

## console-api-v0.7.0 - (2024-06-10)

### <a id = "0.7.0-breaking"></a>Breaking Changes
- **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic` dependency to a
semver-incompatible version. This breaks compatibility with `tonic`
0.10.x.

### Documented

- Fix typo in proto ([#472](#472)) ([2dd3559](2dd3559))

### Updated

- [**breaking**](#0.7.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c))


## `console-subscriber`

## console-subscriber-v0.3.0 - (2024-06-10)

### <a id = "0.3.0-breaking"></a>Breaking Changes
- **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic` dependency to a
semver-incompatible version. This breaks compatibility with `tonic`
0.10.x.

### Added

- Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa))
- Reduce retention period to fit in max message size ([#503](#503)) ([bd3dd71](bd3dd71))
- Support grpc-web and add `grpc-web` feature ([#498](#498)) ([4150253](4150253))

### Documented

- Add a grpc-web app example ([#526](#526)) ([4af30f2](4af30f2))
- Fix typo in doc comment ([#543](#543)) ([ff22678](ff22678))

### Fixed

- Don't save poll_ops if no-one is receiving them ([#501](#501)) ([1656c79](1656c79))
- Ignore metadata that is not a span or event ([#554](#554)) ([852a977](852a977))

### Updated

- [**breaking**](#0.3.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hds hds hds approved these changes

@hawkw hawkw hawkw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbiggio @hds @hawkw