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

Update query console right side panel in Fleet product #7080

Closed
noahtalerman opened this issue Aug 5, 2022 · 36 comments
Closed

Update query console right side panel in Fleet product #7080

noahtalerman opened this issue Aug 5, 2022 · 36 comments
Assignees
Labels
~frontend Frontend-related issue. ~legacy-interface-product-group Associated with the legacy "interface" product group. (No longer exists)
Milestone

Comments

@noahtalerman
Copy link
Member

noahtalerman commented Aug 5, 2022

Goal

Update query console right side panel to improve first-time experiences with Fleet and make it more approachable to new users.

Figma

https://www.figma.com/file/yLP0vJ8Ms4GbCoofLwptwS/%E2%9C%85-fleetdm.com-(current%2C-dev-ready)?node-id=5197%3A20730

Requirements

Related

Notes

Tasks

  • TODO: Update with implementation requirements.
@noahtalerman noahtalerman added ~legacy-interface-product-group Associated with the legacy "interface" product group. (No longer exists) 🏆 OKR labels Aug 5, 2022
@noahtalerman noahtalerman self-assigned this Aug 9, 2022
@lukeheath lukeheath assigned ghernandez345 and unassigned lukeheath Aug 29, 2022
@lukeheath
Copy link
Member

@ghernandez345 I'm assigning you this ticket from the roadmap to begin the spec process. Please spend some time researching the parent epic and related issues to understand goals and context.

Please also review the attached Figma to understand the visual changes. Make sure to read all of the dev notes, as they contain important details.

When you have a clear understanding of how this feature will be implemented, update the "Tasks" list to identify the tasks necessary to complete this issue.

If you have any ideas, questions, or concerns, please tag the appropriate people in a comment on this issue. Thanks!

@lukeheath
Copy link
Member

@ghernandez345 One consideration for this feature is how to order the work between the schema work that Eric is doing and the interface work this ticket covers. Can we build it in unison, or should we wait for the schema work to be finished?

@noahtalerman
Copy link
Member Author

@mike-j-thomas it looks like there's a remaining wireframe TODO. If we still want this tooltip, can you please add this tooltip to the wireframes?

Image

@noahtalerman
Copy link
Member Author

cc @ghernandez345 ^

@noahtalerman noahtalerman changed the title In Fleet product, update query console right side panel Update query console right side panel in Fleet product Sep 1, 2022
@ghernandez345
Copy link
Contributor

@noahtalerman @mike-j-thomas what does "anything else if relevant" mean here? are there other OS platforms that could possibly be displayed? which are those and do we have icons we want to show with them?

Image

@ghernandez345
Copy link
Contributor

ghernandez345 commented Sep 6, 2022

@noahtalerman @zhumo I think we need to add the data to the fleet schema for columns that default to root in order to implement this on the UI. I dont see anything on the columns objects that would give us this atm.

Image

@ghernandez345
Copy link
Contributor

@noahtalerman can I get more clarity about what the dev note about replace newlines with spaces means?

image

@ghernandez345
Copy link
Contributor

ghernandez345 commented Sep 6, 2022

@noahtalerman @zhumo looking at the fleet schema we have examples as plural.

"examples":"// This will get everything\nSELECT * FROM account_policy_data;"

does this mean we could possibly have multiple query examples for a table? if so how do you feel about the data structure looking more like this?

"examples": {
  "description": "This will get everything",
  "queries": ["SELECT * FROM account_policy_data;", "SELECT * FROM account_policy_data WHERE uid = 1;"]
}

@ghernandez345
Copy link
Contributor

@ghernandez345 One consideration for this feature is how to order the work between the schema work that Eric is doing and the interface work this ticket covers. Can we build it in unison, or should we wait for the schema work to be finished?

@lukeheath i think we can work in parallel when building. as long as we have an agreed contract of how the schema will look and it has all the data we will need. Of course, we'll need the schema implemented to release to prod though

@mike-j-thomas
Copy link
Member

mike-j-thomas commented Sep 6, 2022

@ghernandez345

can I get more clarity about what the dev note about replace newlines with spaces means?

On the website version, the SQL is easier to view when broken into new lines.
image

But we don't have much space in the product sidebar, so to reduce scrolling, new lines should be replaced by a space instead. So the SQL will appear wrapped.

image

I updated the wireframes to better show the wrapped SQL. Sorry for the confusion.

@ghernandez345
Copy link
Contributor

ghernandez345 commented Sep 6, 2022

@ghernandez345

can I get more clarity about what the dev note about replace newlines with spaces means?

On the website version, the SQL is easier to view when broken into new lines. image

But we don't have much space in the product sidebar, so to reduce scrolling, new lines should be replaced by a space instead. So the SQL will appear wrapped.

ah ok. @noahtalerman @zhumo will the new fleet schema include new line characters within the example queries? in the example query there are none included atm. to display the queries in s smaller space its fine if the queries dont have newline characters

"examples": "// This will get everything\nSELECT * FROM account_policy_data;"

@zhumo
Copy link
Contributor

zhumo commented Sep 6, 2022

@ghernandez345

I don't believe there are any more OS icons. We pared down to mac, windows, and linux a release or two ago.

Yes, there will be individual attribute defaults to root in the example schema.

The examples attribute is intended to be a blank open text field which accepts markdown. It will/should be joined into a sentence that supports newlines. Do the newlines need to be explicitly written or can it be inferred? @mike-j-thomas @eashaw

@eashaw
Copy link
Contributor

eashaw commented Sep 6, 2022

@zhumo Newlines should be explicitly written as \n

@lukeheath
Copy link
Member

@ghernandez345 Heads up; you are now unblocked on this. @eashaw hooked us up with a file at https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json that he will be updating every three weeks as part of the digital experience rituals. Let us know if you run into any blockers. Thanks!

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 11, 2022

@noahtalerman @zhumo @eashaw I'm looking through the schema here https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json and do not see any instances of requires_user_context on any of the columns. Is this correct? If so can we safely assume columns will never have that property?

Also if this is the case, In the UI you will never see the Defaults to root messaging in the updated sidebar.

yes, "defaults to root" is requires_user_context.

Is this correct or has "defaults to root" been associated with a different column property now?

image

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 11, 2022

@eashaw I also notice we use different values for platform names for a table and its columns

e.g

{
  // for a table
  platforms: [
    "darwin",
    "windows",
    "linux",
  ],

  // for columns
  columns: [
    {
      platforms: [
        "macOS",
        "Windows",
        "Linux",
      ]
  ]
}

is it possible that we only use darwin, windows, and linux for consistency? It also leads to less checking on the frontend for additional values.

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 11, 2022

@noahtalerman there seems to be a new platform type named freebsd.

{
    ....
    "platforms": [
      "darwin",
      "linux",
      "windows",
      "freebsd"
    ],
    ....
}

Do we have an icon we want to use to display this in the UI?

Image

@noahtalerman
Copy link
Member Author

noahtalerman commented Oct 11, 2022

Hmm, we don't want to display FreeBSD because osquery no longer supports it. Context is here: #4507

@ghernandez345 where did you see freebsd show up?

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 11, 2022

Hmm, we don't want to display FreeBSD because osquery no longer supports it. Context is here: #4507

@ghernandez345 where did you see freebsd show up?

In our new osquery_fleet_schema.json. This is the data that will power the website and new sidebar. https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json. @eashaw is there any issue with removing this platform from this schema?

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 11, 2022

@zhumo @eashaw I noticed that the strings for the examples do not match the proposed schema. They seem to add ```\n characters around the bit that is supposed to be the query.

e.g.

"examples": "See the domain, if any, that the Mac is bound to.\n```\nSELECT domain FROM ad_config;\n```"

This differs from the schema requirements in this issue https://github.com/fleetdm/confidential/issues/1619

"examples":"This will get everything\nSELECT * FROM account_policy_data;", # supports markdown

Is it possible to remove the ```\n characters as the web UI has been built with that string format in mind? Currently, the examples look like this.

image.

Somewhat related I also notice that we can have multiple examples for a table, but did not account for that in our schema example. I see from the data in the schema that we've solved this with this set of characters```\n\n. As we are generating this schema ourselves, could we just change it so the data is more intuitive to work with. Something like

{
  examples: [
    {
        description: "description 1",
        query: "SELECT * "
    },
    {
        description: "description 2",
        query: "SELECT * "
    }
  ],
}

I think not only is it more intuitive but easier to maintain, easier to add more examples, and less likely to lead to bugs from having to parse a complicated string.

@eashaw
Copy link
Contributor

eashaw commented Oct 11, 2022

@noahtalerman @zhumo @eashaw I'm looking through the schema here https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json and do not see any instances of requires_user_context on any of the columns. Is this correct? If so can we safely assume columns will never have that property?

Also if this is the case, In the UI you will never see the Defaults to root messaging in the updated sidebar.

yes, "defaults to root" is requires_user_context.

Is this correct or has "defaults to root" been associated with a different column property now?

image

@ghernandez345 This is correct "defaults to root" is requires_user_context.

is it possible that we only use darwin, windows, and linux for consistency? It also leads to less checking on the frontend for additional values.

@ghernandez345 Yes! The column platform names are changed to the other values in get-extended-osquery-schema.js to add "Only available on _____ " to column descriptions. I can make a PR to update the behavior to only do this when merging the schema to build Markdown pages.

@zhumo @eashaw I noticed that the strings for the examples do not match the proposed schema. They seem to add ```\n characters around the bit that is supposed to be the query.

e.g.

"examples": "See the domain, if any, that the Mac is bound to.\n```\nSELECT domain FROM ad_config;\n```"

This differs from the schema requirements in this issue fleetdm/confidential#1619

"examples":"This will get everything\nSELECT * FROM account_policy_data;", # supports markdown

Is it possible to remove the ```\n characters as the web UI has been built with that string format in mind? Currently, the examples look like this.

image.

Somewhat related I also notice that we can have multiple examples for a table, but did not account for that in our schema example. I see from the data in the schema that we've solved this with this set of characters```\n\n. As we are generating this schema ourselves, could we just change it so the data is more intuitive to work with. Something like

{
  examples: [
    {
        description: "description 1",
        query: "SELECT * "
    },
    {
        description: "description 2",
        query: "SELECT * "
    }
  ],
}

I think not only is it more intuitive but easier to maintain, easier to add more examples, and less likely to lead to bugs from having to parse a complicated string.

@ghernandez345 I think your suggested format makes sense, and it would be relatively easy to update the build script to use it instead of the current string format.
@zhumo @GuillaumeRoss What are your thoughts on this change?

@zhumo
Copy link
Contributor

zhumo commented Oct 11, 2022

@ghernandez345

do not see any instances of requires_user_context on any of the columns. Is this correct?

Sharp eye! requires_user_context is not a column attribute that is native to osquery. Instead, osquery specifies it by table. the requires_user_context is manually added in by us in the fleet schema to be column-specific. This way, we can tell the user which column is the relevant one.

@zhumo
Copy link
Contributor

zhumo commented Oct 11, 2022

@ghernandez345 I would be supportive of always having the platform name be lowercase as you're suggesting, but do you know which source is generating the capitalized one and which source is generating the downcased one?

@zhumo
Copy link
Contributor

zhumo commented Oct 11, 2022

@ghernandez345 freebsd has been removed from the schema after the next osquery version. When we get a new schema generated, that will naturally disappear from the schema. Up to you guys whether you want to remove it or will just ignore it.

@zhumo
Copy link
Contributor

zhumo commented Oct 11, 2022

@ghernandez345 @eashaw It has been a huge pain putting in that ```\n\n stuff. However, I would like to try to maintain some amount of flexibility for markdown rather than get locked into the strict "description/example" approach you've proposed. Maybe one step back could be that the examples became an array of strings and the system will auto-join them with \n. So:

{
  examples: [
    "My description of my query.",
    "", # Empty string for an empty line
    "```",
    "SELECT *",
    etc. etc.
  ]
}

This both preserves flexibility, while making it easier to read.

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 12, 2022

@zhumo @eashaw , I had a discussion with @lukeheath about some of our options for implementing this UI and since we want to get this out in this release I'm going to change my code to match the current osquery_fleet_schema.json. So no need to make any changes on your side atm.

@eashaw I do think we need to correct the example string for the keychain_acls. It is missing the \n```\n characters when separating the description from the example.

{"examples": "Identify keychain items with permissions granted to Applications at the system or user level.SELECT * FROM keychain_acls WHERE path LIKE '/System/Applications/%%' OR path LIKE '/Users/%%/Applications/%%';\n```\nSELECT * FROM keychain_acls WHERE path LIKE '/System/Applications/%%' OR path LIKE '/Users/%%/Applications/%%';\n```"}

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 12, 2022

@noahtalerman @zhumo @eashaw Ok so I've tried to update my UI from the new data query and I noticed inconsistent data for the examples attribute that will need to be fixed on the schema generation side. I need the data to be consistent to correctly parse and display this data in the UI. here are a couple of examples.

how most examples are

"examples": "Select all users.\n```\nSELECT * FROM users;\n```"

missing first set of separation characters between description and first query

"examples": "Select all users.SELECT * FROM users;\n```"

missing final separation characters at the end, seems to happen with tables with multiple examples e.g. `account_policy_data

"examples": "Select all users.\n```\nSELECT * FROM users;\n```\nSELECT * FROM users;"

@noahtalerman
Copy link
Member Author

@zhumo heads up, this likely won't make it into the next release (4.22). Code freeze starts EOD Friday (2022-10-14).

My current understanding on what's left:

To wrap up the UI, we need the updates to the schema that @ghernandez345 called out in this comment: #7080 (comment)

@zhumo
Copy link
Contributor

zhumo commented Oct 12, 2022

@ghernandez345 why do these formatting issues block your work? They are still valid Markdown right? They should still render as long as you feed it into the markdown parser.

@xpkoala
Copy link
Contributor

xpkoala commented Oct 18, 2022

A few small items I've found that could use some help.

  1. The ordering of the operating systems is out of spec with Figma. The ordering should be macOS, Windows, Linux.
  2. Some keys can overrun the space provided and push the data type.
  3. When the notes section shows syntax there is an extra line added that should be omitted

Pics:
1.
Screenshot 2022-10-18 at 2 39 37 AM

Screenshot 2022-10-18 at 2 37 28 AM

Screenshot 2022-10-18 at 2 36 05 AM

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 18, 2022

@noahtalerman @mike-j-thomas For this issue I was thinking we can line break these long column names. What do you think? I tried line break but the UX is annoying as users arent able to copy the whole column name.

before

Screenshot 2022-10-18 at 2 37 28 AM

After

image

doesn't look amazing but I think this case is rare and has better UX then truncating

@noahtalerman
Copy link
Member Author

I tried line break but the UX is annoying as users arent able to copy the whole column name.

@ghernandez345 did you mean "truncate" instead of "line break" in your message above?

Line breaking the long column names makes sense! I think this is a great solution. Prioritizing the ability to copy makes sense.

The tooltip indicator is a bit awkward for the two line column names. It hangs past the 2nd line. We can address this in a later pass.

cc @mike-j-thomas

@ghernandez345
Copy link
Contributor

ghernandez345 commented Oct 18, 2022

@noahtalerman yep, my bad. I tried both truncated and linebreak and linebreak had a better UX imo

@mike-j-thomas
Copy link
Member

Good catch, @ghernandez345. Yeah, truncating would look much better, but if it is hindering copy and pasting, then I agree that line-breaking is the better solution. It does look quite rough though. Is it possible to at least break the line after an "_"? That would help with readability.

Example:

memory_array_mapped_
address_handle

cc @noahtalerman

@mike-j-thomas
Copy link
Member

mike-j-thomas commented Oct 19, 2022

@ghernandez345, the truncated text interfering with the chevron in the dropdown also needs addressing, albeit a different issue.
image

@noahtalerman
Copy link
Member Author

Thanks Mike!

@ghernandez345 do you think it makes sense to pull the above fixes into the "User interface polish pass (v1) issue? #8238

This way, they're tracked.

@lukeheath lukeheath added this to the 4.22.0 milestone Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
~frontend Frontend-related issue. ~legacy-interface-product-group Associated with the legacy "interface" product group. (No longer exists)
Development

No branches or pull requests

7 participants