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

chore: RFC #1999 - 2020-03-06 - API extensions for lua transform #2000

Merged
20 commits merged into from
Mar 17, 2020
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 204 additions & 0 deletions rfcs/2020-03-06-1999-api-extensions-for-lua-transform.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
# RFC #1999 - 2020-03-06 - API extensions for `lua` transform

This RFC proposes a new API for the `lua` transform.

## Motivation

Currently, the [`lua` transform](https://vector.dev/docs/reference/transforms/lua/) has some limitations in its API. In particular, the following features are missing:

* **Nested Fields**

Currently accessing nested fields is possible using the dot notation:

```lua
event["nested.field"] = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can expose this functionality in a .get API call or something?

Copy link
Author

Choose a reason for hiding this comment

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

We can. There are some intricacies, for example if it is added as a method, it would not work with new user-created events which are tables:

event = {
  log = {
    -- ...
  }
}
emit(event)

Here event is not a userdata, but a table, so it would not have a get method.

However, it might be possible to provide a global function for accessing this, although see #2000 (comment).

```

However, users expect nested fields to be accessible as native Lua structures, for example like this:

```lua
event["nested"]["field"] = 5
```

See [#706](https://github.com/timberio/vector/issues/706) and [#1406](https://github.com/timberio/vector/issues/1406).

* **Setup Code**

Some scripts require expensive setup steps, for example, loading of modules or invoking shell commands. These steps should not be part of the main transform code.

For example, this code adding custom hostname

```lua
if event["host"] == nil then
local f = io.popen ("/bin/hostname")
local hostname = f:read("*a") or ""
f:close()
hostname = string.gsub(hostname, "\n$", "")
event["host"] = hostname
end
```

Should be split into two parts, the first part executed just once at the initialization:

```lua
local f = io.popen ("/bin/hostname")
local hostname = f:read("*a") or ""
f:close()
hostname = string.gsub(hostname, "\n$", "")
```

and the second part executed for each incoming event:

```lua
if event["host"] == nil then
event["host"] = hostname
end
```

See [#1864](https://github.com/timberio/vector/issues/1864).

* **Control Flow**

It should be possible to define channels for output events, similarly to how it is done in [`swimlanes`](https://vector.dev/docs/reference/transforms/swimlanes/) transform.

See [#1942](https://github.com/timberio/vector/issues/1942).

## Prior Art

The implementation of `lua` transform has the following design:

* There is a `source` parameter which takes a string of code.
* When a new event comes in, the global variable `event` is set inside the Lua context and the code from `source` is evaluated.
* After that, Vector reads the global variable `event` as the processed event.
* If the global variable `event` is set to `nil`, then the event is dropped.

Events have type [`userdata`](https://www.lua.org/pil/28.1.html) with custom [metamethods](https://www.lua.org/pil/13.html), so they are views to Vector's events. Thus passing an event to Lua has zero cost, so only when fields are actually accessed the data is copied to Lua.

The fields are accessed through string indexes using [Vector's dot notation](https://vector.dev/docs/about/data-model/log/#dot-notation).

## Guide-level Proposal

### Motivating example


```toml
[transforms.lua]
type = "lua"
inputs = []
version = "2" # defaults to 1
source = """
counter = counter + 1
-- without calling `emit` function nothing is produced by default
"""
[transforms.lua.hooks]
init = """
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm, I originally considered functions (Eg start, stop) but this works too!

Copy link
Contributor

Choose a reason for hiding this comment

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

Notably, if we want a consistent interface for lua, javascript, and later wasm this may not be the best choice. It may be better to let folks specify some functions as hooks instead.

Copy link
Author

Choose a reason for hiding this comment

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

Initially I was thinking about an API looking like this:

source = """
  function (input, emit)
    input:on_event(function (event)
      -- do something
      emit(event)
    end)
    input:on_start(function ()
      event = -- ...
      emit(event)
    end)
    input:on_stop(function ()
      -- ...
    end)
    input:on_interval(10, function()
      event = -- ...
      emit(event)
    end)
  end
"""

However, if such a script is a part of an already large TOML config file, such as the one from #1721 (comment), declaring the hooks using the TOML syntax might make it easier to read and reason about. It limits generality, but in return it makes it easier to both get started and reason about scripts written by other people, especially if the user is not deeply familiar with Lua programming.

In someone needs for some reason to put functions as hooks, it is possible too:

hooks.start = """
require "mymodule"
function timer_handler(emit)
-- ...
end
my_processor = mymodule.create_processor()
"""
timers = [{ interval = 10, source = "timer_handler(emit)" }]
source = "my_processor:process(event, emit)"

Copy link
Contributor

Choose a reason for hiding this comment

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

However, if such a script is a part of an already large TOML config file, such as the one from #1721 (comment), declaring the hooks using the TOML syntax might make it easier to read and reason about.

In the context of the vector.toml file I tend to agree. But I could also see users wanting to include separate .lua files, which makes this point moot. For example:

source = """
require("transform.lua")
"""

That said, what if allowed the user to specify handlers instead of source code? Ex:

[transforms.lua]
  type = "lua"
  source = """
counter = 0

function init
  # ...
end

function process(event)
  # ...
end
"""

  handlers.init = "init" # default 
  handlers.process = "process" # default
  handlers.shutdown = "shutdown" # default 

  timers = [
    {handler = "flush", interval_secs = 10}
  ]

This would then allow a user to do something like:

[transforms.lua]
  type = "lua"
  source = "require('transform.lua')"

  handlers.init = "init" # default 
  handlers.process = "process" # default
  handlers.shutdown = "shutdown" # default 

  timers = [
    {handler = "flush", interval_secs = 10}
  ]

Copy link
Contributor

@Hoverbear Hoverbear Mar 9, 2020

Choose a reason for hiding this comment

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

@a-rodin Right, so in your example each of those scripts shares a global state? I was under the initial impression they were distinct.

How would this work if the language didn't work well with random mutable global variables, like Rust? Would we just be concatting them?

Copy link
Author

@ghost ghost Mar 10, 2020

Choose a reason for hiding this comment

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

@binarylogic I think this approach suits both simple and advanced use cases. I’m going to update the RFC text.

@Hoverbear
I’m not sure what is the best approach for Rust, but I think that it might be a solution to define a trait with methods init, process, timers, shutdown which would then be implemented by the users (or have default implementations for methods like init/shutdown hooks). So the execution model could still be the same, although the configuration would have to appear somewhat differently because it is not practical to write inline Rust in the config files.

On the other hand, as Lua is an interpreted language, for light transforms it would be possible to just write inline Lua functions with the current approach, and the trait-like approach would require writing unnecessary boilerplate code and still be not idiomatic.

counter = 0
previous_timestamp = os.time()
emit({
log = {
message = "starting up",
timestamp = os.date("!*t),
}
}, "auxiliary")
"""
shutdown = """
final_stats_event = {
log = {
count = counter,
timestamp = os.date("!*t"),
interval = os.time() - previous_timestamp
}
}
final_stats_event.log.stats.rate = final_stats_event["log"]["stats"].count / final_stats_event.log.stats.interval
emit(final_stats_event)

emit({
log = {
message = "shutting down",
timestamp = os.date("!*t"),
}
}, "auxiliary")
"""
[[transforms.lua.timers]]
interval = 10
source = """
emit {
metric = {
name = "response_time_ms",
timestamp = os.date("!*t"),
kind = "absolute",
tags = {
host = "localhost"
},
value = {
type = "counter",
value = 24.2
}
}
}
"""
[[transforms.lua.timers]]
interval = 60
source = """
event = {
log = {
message = "heartbeat",
timestamp = os.date("!*t),
}
}
emit(event, "auxiliary")
"""
```

The code above consumes the incoming events, counts them, and then emits these stats about these counts every 10 seconds. In addition, it sends debug logs about its functioning into a separate lane called `auxiliary`.

### Proposed changes

* Add `version` configuration option which would allow the users to chose between the new API described in this RFC (version 2) and the old one (version 1).
* Hooks for initialization and shutdown called `init` and `shutdown`. They are defined as strings of Lua code in the `hooks` section of the configuration of the transform.
* Timers which define pieces of code that are executed periodically. They are defined in array `timers`, each timer takes two configuration options: `interval` which is the interval for execution in seconds and `source` which is the code which is to be executed periodically.
* Events are produced by the transform by calling function `emit` with the first argument being the event and the second option argument being the name of the lane where to emit the event. Outputting the events by storing them to the `event` global variable should not be supported, so its content would be ignored.
* Support direct access to the nested fields (in both maps and arrays).
* Add support for the timestamp type as a `userdata` object with the same visible fields as in the table returned by [`os.date`](https://www.lua.org/manual/5.3/manual.html#pdf-os.date). In addition, monkey-patch `os.date` function available inside Lua scripts to make it return the same kind of userdata instead of a table if it is called with `*t` or `!*t` as the argument. This is necessary to allow one-to-one correspondence between types in Vector and Lua.

## Sales Pitch

The proposal

* gives users more power to create custom transforms;
* supports both logs and metrics;
* makes it possible to add complexity to the configuration of the transform gradually only when needed.

## Drawbacks

The only drawback is that supporting both dot notation and classical indexing makes it impossible to add escaping of dots in field names. For example, for incoming event structure like
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I'm in favor of not supporting the dot notation within runtimes, even if this is a breaking change. This syntax was one of the reasons that prompted a refactoring of our internal model, and dot notation should only be used in circumstances where a field must be accessed with a string (eg not in a runtime).


```json
{
"field.first": {
"second": "value"
}
}
```

accessing `event["field.first"]` would return `nil`.

However, because of the specificity of the observability data, there seems to be no need to have both field names with dots and nested fields.

## Outstanding Questions

* Should timestamps be automatically inserted to created logs and metrics created as tables inside the transform is they are not present?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why they should...

Copy link
Author

Choose a reason for hiding this comment

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

For example, stdin source inserts timestamps to newly created events automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should not be possible for a source to emit an event without a timestamp field, so the lua transform should not concern itself with this. If a user explicitly removes the timestamp field then they'll deal with problems, but we should not cater to that edgecase.

Copy link
Author

Choose a reason for hiding this comment

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

The question is about newly created events inside the transforms, as the user can forget to add the timestamp to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Yes, in that case, we should populate the appropriate schema fields. The names should respect those provided as part of global log_schema setting. I'd argue that both timestamp and host should be automatically set.

* Are there better alternatives to the proposed solution for supporting of the timestamp type?
* Could some users be surprised if the transform which doesn't call `emit` function doesn't output anything?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a possibility, but I think docs are sufficient here.

* `null` might present in the events would be lost because in Lua setting a field to `nil` means deletion. Is it acceptable? If it is not, it is possible to introduce a new kind of `userdata` for representing `null` values.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your take on this @binarylogic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's odd, but I think this is fine since it is a common Lua behavior.


## Plan of Action

- [ ] Implement support for `version` config option and split implementations for versions 1 and 2.
- [ ] Implement access to the nested structure of logs events.
- [ ] Support creation of logs events as table inside the transform.
- [ ] Implement metrics support.
- [ ] Add `emit` function.
- [ ] Add `init` and `shutdown` hooks.
- [ ] Add timers.
- [ ] Implement support for the timestamp type compatible with the result of execution of `os.date("!*t")`.