-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add ability to use existing processors from script processor #11260
Add ability to use existing processors from script processor #11260
Conversation
2b5dd72
to
deaa271
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really minor changes, I've inspect the added goja code too.
} | ||
|
||
func newAddLocale(c *common.Config) (processors.Processor, error) { | ||
// New constructs a new add_locale processor. | ||
func New(c *common.Config) (processors.Processor, error) { | ||
config := struct { | ||
Format string `config:"format"` | ||
}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice little refactoring.
|
||
import ( | ||
"github.com/dop251/goja" | ||
"github.com/dop251/goja_nodejs/require" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've inspected the package from goja everything is clean 👍
|
||
log := logp.L().Named("processor.javascript") | ||
switch level { | ||
case logp.DebugLevel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we missing Info
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you brought this up. For some reason I had it in my head that the typical console API in the browser didn't have an info
. But I was wrong. https://developer.mozilla.org/en-US/docs/Web/API/Console/info
I'll add this.
o.Set("log", c.makeLogFunc(logp.DebugLevel)) | ||
o.Set("warn", c.makeLogFunc(logp.WarnLevel)) | ||
o.Set("error", c.makeLogFunc(logp.ErrorLevel)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we are trying to add a convenience debug method of console, It's bad that console doesn't completely map to the our Error Level, we might want to consider to alias info()
, debug()
to log()
.
Or by looking at the code we don't allow a user to use the info level, I am wondering if we should permit that by either allow info()
to log to the info level. official doc
Another way would be to expose logp
as new module with the method mapped to the right level.
} | ||
` | ||
|
||
logp.TestingSetup(logp.ToObserverOutput()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
return b.volAndPath[:b.volLen+b.w] | ||
} | ||
return b.volAndPath[:b.volLen] + string(b.buf[:b.w]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add unit test for the windows path handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some tests in path_test.go
for Windows.
|
||
func (bp *beatProcessor) Run(call goja.FunctionCall) goja.Value { | ||
if len(call.Arguments) != 1 { | ||
panic(bp.runtime.NewGoError(errors.Errorf("Run requires one argument"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run -> run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run
is the name of the function that's exposed in the JS. That's why it's capitalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have been more precise I was talking about the string user in Errorf
errors.Errorf("Run requires one argument")
// | ||
// // javascript | ||
// var processor = require('processor'); | ||
// var chopLog = new processor.Dissect({tokenizer: "%{key}: %{value}"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get to the choppa https://www.youtube.com/watch?v=Xs_OacEq2Sk
@andrewkroh you will need to run |
This adds a `require` function to the JS runtime that scripts can call to import "modules". Some standard modules that are added in this PR are - `processor` - You can construct beat processors in JS (e.g. `new processor.Dissect({...})`). - `console` - You can write to beat's logger. - `path` - You can parse win32 and posix paths. The `processor` module supports constructing: - `AddCloudMetadata` - `AddDockerMetadata` - `AddHostMetadata` - `AddKubernetesMetadata` - `AddLocale` - `AddProcessMetadata` - `CommunityID` - `DecodeJSONFields` - `Dissect` - `DNS`
The module registry is a global so 'console' is only initialized once. But if logp is reinitialized the stored logger would have the old settings. So don't use a global logger.
98fdf8b
to
c3637a5
Compare
@ph So I added |
@andrewkroh changes look good, only the Errorf message and there is still a test failling on travis.
|
Use logp.DevelopmentSetup and not TestingSetup.
I'm pretty sure I figured out the problem with the test now. I should really read the godocs that I wrote next time. 🤦♂️ |
This adds a
require
function to the JS runtime that scripts can call to import"modules". The modules that are added in this PR are
processor
- You can construct beat processors in JS (e.g.new processor.Dissect({...})
).console
- You can write to beat's logger.path
- You can parse win32 and posix paths.The
processor
module supports constructing:AddCloudMetadata
AddDockerMetadata
AddHostMetadata
AddKubernetesMetadata
AddLocale
AddProcessMetadata
CommunityID
DecodeJSONFields
Dissect
DNS
Basic Example
This is a basic example. It's nothing you couldn't do already with the YAML format, but imagine you need to start adding in some more complex conditions and following some conditional branches.