Skip to content

(web) Add term input field to filters bar to filter resource logs by string#4608

Merged
lizzthabet merged 3 commits into
masterfrom
lizz/feature/filter-logs-by-string
Jun 16, 2021
Merged

(web) Add term input field to filters bar to filter resource logs by string#4608
lizzthabet merged 3 commits into
masterfrom
lizz/feature/filter-logs-by-string

Conversation

@lizzthabet

@lizzthabet lizzthabet commented Jun 2, 2021

Copy link
Copy Markdown
Contributor
  • Add support for filtering log spans based on a string input that behaves like the existing source and term filters
  • Add a term input field in the filter bar with a debounce delay; (this does behave a tiny bit quirky which i outlined in a code note. I think this is probably okay for mvp, but happy to chat more in-depth if needed.)

Styles and tests are still a wip, but otherwise code is ready for review.

Term input field:
Screen Shot 2021-06-02 at 5 47 34 PM

Filtering logs by term and level filters:
2021-06-02 17 53 26

Closes ticket #12046

@lizzthabet
lizzthabet requested review from hyu, landism and nicks June 2, 2021 22:12
@lizzthabet lizzthabet self-assigned this Jun 2, 2021
@lizzthabet
lizzthabet marked this pull request as draft June 2, 2021 22:12
Comment thread web/src/OverviewLogPane.tsx Outdated
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from 0eef42e to e7c643a Compare June 3, 2021 17:16
Comment thread web/src/OverviewLogPane.tsx Outdated

@landism landism left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The epic and docs say:

  1. you can filter by logs span EXCLUDING [term]
  2. the search term is a raw string unless surrounded by //, in which case it's a regexp

Are these still planned?

Comment thread web/src/SrOnly.tsx Outdated
Comment thread web/src/logfilters.ts Outdated
Comment thread web/src/logfilters.ts Outdated
Comment thread web/src/OverviewActionBar.tsx Outdated
Comment thread web/src/OverviewActionBar.tsx Outdated
Comment thread web/src/OverviewLogPane.tsx
Comment thread web/src/OverviewLogPane.tsx Outdated
@lizzthabet

lizzthabet commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

The epic and docs say:

1. you can filter by logs span EXCLUDING [term]

2. the search term is a raw string _unless_ surrounded by `//`, in which case it's a regexp

Are these still planned?

They are! I broke the epic into smaller stories, so this first one is #12046 and just covers the string search. I was planning to do the other tickets (including regexp) in the epic as fast follows. (But also lmk if there's a more preferable way for me to merge in this work.)

@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from e7c643a to 2c9a431 Compare June 7, 2021 16:32
Comment thread web/src/logfilters.ts Outdated
@lizzthabet
lizzthabet marked this pull request as ready for review June 9, 2021 18:59
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from 070205a to 027dea5 Compare June 9, 2021 19:21
@lizzthabet
lizzthabet requested a review from landism June 9, 2021 19:27
Comment thread web/src/OverviewActionBar.stories.tsx
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from 027dea5 to e21d9d7 Compare June 10, 2021 15:22

@landism landism left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking pretty good!

Comment thread web/src/OverviewActionBar.tsx Outdated
Comment thread web/src/OverviewActionBar.tsx Outdated
Comment thread web/src/OverviewActionBar.tsx
Comment thread web/src/OverviewActionBar.tsx Outdated
Comment thread web/src/OverviewActionBar.tsx
Comment thread web/src/OverviewActionBar.tsx

@hyu hyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! It took me a little while to read through this code, and I think the PR would have been a little easier to understand if it was somehow split into more PRs. (e.g., separate PR for RegEx functionality / debouncing). I appreciate your comments, like those on debouncing, since that did help me understand the code better.

I added a note re: a fix for the width change on the input that Matt also noticed.

Comment thread web/src/OverviewActionBar.tsx
@lizzthabet
lizzthabet marked this pull request as draft June 11, 2021 18:36
@lizzthabet
lizzthabet marked this pull request as ready for review June 14, 2021 21:49
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch 2 times, most recently from 2a83c83 to a62da6c Compare June 15, 2021 15:29
@lizzthabet
lizzthabet requested a review from hyu June 15, 2021 15:33
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from a62da6c to 7584da5 Compare June 15, 2021 16:11
@lizzthabet
lizzthabet force-pushed the lizz/feature/filter-logs-by-string branch from 7584da5 to 5b666cf Compare June 16, 2021 15:17
@lizzthabet
lizzthabet merged commit 731d140 into master Jun 16, 2021
@lizzthabet
lizzthabet deleted the lizz/feature/filter-logs-by-string branch June 16, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants