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

Zed JS #1603

Merged
merged 44 commits into from
May 4, 2021
Merged

Zed JS #1603

merged 44 commits into from
May 4, 2021

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented May 3, 2021

The full zed type system in JavaScript and all the implications of the new apis in the app.

  • The Zed type system is located in zealot/zed. There are classes for each type and for each value. The context classes handles decoding and encoding into and from zjson.
  • Each process in the app will use a singleton ZealotContext to decode and encode any Zed data it sees.
  • A "responses" system has been created to make it easy to re-generate zqd responses if things change again in the future. See test/responses/index.ts
  • The log cell renderer has been updated and simplified to render complex fields like Arrays and Sets. See app/viewer/value.tsx and app/viewer/cell.tsx.
  • The rightClick menu build prop drilling is gone, in favor of a simple dispatched thunk 🎉. (Some dangling code probably needs to be deleted)
  • The contextMenu integration test has been refactored and updated.
  • The createCell object has been removed and replaced with other simpler functions.
  • Record serialization now looks like ZealotContext.encodeRecord(r) and ZealotContext.decodeRecord(zjson)
  • Encoding a single field is ZealotContext.encodeField(f) and ZealotContext.decodeField(f)

Happy to answer any other questions as the come up!

Fixes #1608
Fixes #832

@jameskerr jameskerr requested a review from mccanne May 4, 2021 00:06
@jameskerr jameskerr requested a review from mason-fish May 4, 2021 00:06
Copy link
Contributor

@mccanne mccanne left a comment

Choose a reason for hiding this comment

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

I don't understand all the react etc pieces, but the zealot/zed stuff looks good to me. At some point, we'll want more comprehensive treatment of vaulues (e.g., 64-bit. precision etc)

if (zed.isInt(data)) return withCommas(data.toString())
if (zed.isTime(data)) return brim.time(data.toDate()).format()
if (zed.isStringy(data)) {
// only whitespace
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't doing anything...

null
]
const record = new zng.Record(type, value)
const record = createRecord({
Copy link
Contributor

Choose a reason for hiding this comment

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

Did createRecord() already exist? I think the zed library would have this as a helper method, e.g., CreateRecordFromObject() takes an javascript object, normalizes the keys into an deterministic column order, and looks up the zed.TypeRecorcd, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea.


export function createRecord(object): zed.Record {
let fields: zed.Field[] = []
for (let name in object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you normalize the column order or does javascript semantics these days do the right thing? The main thing is to look up the same zng type no matter what the column order is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most javascript engines keep the order, but it's not a guarantee. I'd like to normalize the order as you suggest eventually. When we move it into the zed package, that would be a good time. This is just a helper function used in unit tests.


export const STRING = {kind: "primitive", name: "string"} as PrimitiveType
export const TIME = {kind: "primitive", name: "time"} as PrimitiveType
export const COUNT = {kind: "primitive", name: "count"} as PrimitiveType
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no primitive in Zed called "count". With zeek import, the count type is turned to uint64.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this whole file was left over from old times. I'll delete.

export const STRING = {kind: "primitive", name: "string"} as PrimitiveType
export const TIME = {kind: "primitive", name: "time"} as PrimitiveType
export const COUNT = {kind: "primitive", name: "count"} as PrimitiveType
export const INTERVAL = {kind: "primitive", name: "interval"} as PrimitiveType
Copy link
Contributor

Choose a reason for hiding this comment

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

Same... no "interval" primtiive.

import {Field} from "./values/field"

export class ZedContext {
private id = 23
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think you needed 23 since it doesn't appear anywhere in the ZJSON protocol, unless I messed up?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is arbitrary. It was convenient to have the same starting id as the backend for testing.

// table, and return it. If it is not, it is an alias. Look it up in the
// context's typeAliases table, and return it.

switch (obj.kind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great! How'd you figure out how to structure this? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

😅

@@ -0,0 +1,54 @@
// export class zed.Primitive {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here all commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to clean that up. It was a reference to the old code. Deleted it.


create(value) {
console.log(value)
throw new Error("Im here!")
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was not used, TypeAlias is the class is actually used for this. Deleted


toInt() {
if (isNull(this.value)) return null
return parseInt(this.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't provide 64-bit resolution

Copy link
Member Author

Choose a reason for hiding this comment

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

I can use BigInt(this.value) to handle the big ints. I tried just now, but it breaks the histogram somehow when trying to sum numbers to gather. I'll follow this PR up with a fix for the 64 bits soon.

@mccanne
Copy link
Contributor

mccanne commented May 4, 2021

I'm deferring to @mason-fish for the approval button.

Copy link
Contributor

@mason-fish mason-fish left a comment

Choose a reason for hiding this comment

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

This lgtm! Left a few comments but am mostly curious about dropping the timestamp format in brimcap. Awesome work wrangling all this together!!

`
const pad = (bool) => (bool ? <Space /> : null)

export default function Value(props: ValueProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

// "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS'Z'"
// )

const tsString = ts.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this still preserve nano second precision? I threw in this js-joda stuff here to help with the formatting since brimcap explicitly expects RFC3999nano

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the time values from zjson are now represent as RFC39999nano strings. I moved the js-joda code in the zed.Time class because I need to parse them there.

const dur = log.get("duration") as zed.Duration
const dest = join(
this.api.getTempDir(),
`packets-${ts.toString()}.pcap`.replaceAll(":", "_")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the fix for Windows you mentioned?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is!

): MenuItemConstructorOptions[] {
const isTime = field.data.getType() === "time"
export default function detailFieldContextMenu({field, record, value}) {
return (_, getState) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

context menus as thunks ftw 🔥

@@ -1,5 +1,6 @@
import typescript from "@rollup/plugin-typescript"
import commonjs from "@rollup/plugin-commonjs"
import nodeResolve from "@rollup/plugin-node-resolve"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this in the dependency changes too, what is it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to pull in the js-joda library into the zealot bundle. We previously had no external dependencies.

@@ -0,0 +1,22 @@
const {Project} = require("ts-morph")
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this sneak in as part of your file renaming PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will delete.

log={log}
style={{width}}
/>
<Cell width={width} key={key} name={field.name}>
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@@ -0,0 +1,5 @@
function load() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just some scaffolding from our convos about the brimcap loaders? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct. Will delete.

@@ -0,0 +1,2 @@
{ first_name: "James" } (=person_schema)
{ score: 100 }
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

const ts = logs[index].get("ts") as zng.Primitive
if (ts.getType() == "time") {
const ts = logs[index].try<zed.Time>("ts")
if (ts && ts instanceof zed.Time) {
Copy link
Member

Choose a reason for hiding this comment

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

Would

       if (ts instanceof zed.Time) {`

suffice here or is the ts && necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it is not needed. Sweet.

@philrz
Copy link
Contributor

philrz commented May 4, 2021

I happened to just try testing a pcap import on this branch (so, commit 6d3f120 at the moment) and it looks like the colored tiles for the different Zeek/Suricata record types have stopped working. @jameskerr is this expected?

@jameskerr
Copy link
Member Author

Thanks for pointing that out. I've put the colors back in.

@jameskerr
Copy link
Member Author

Thanks so much for the reviews everyone. Glad to have this merged. When tests pass, I will merge. But will follow it right up with a proper "duration" parsing function.

Comment on lines -54 to -56
display() {
if (value === "(empty)") {
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just going back in time to recognize that this looks like what fixed #832.

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.

Named types are rejected at import Brim hides the string value "(empty)"
5 participants