Skip to content

enhancement(lua transform): Support metric events in version 2#2095

Merged
4 commits merged intomasterfrom
lua-v2-metrics
Mar 24, 2020
Merged

enhancement(lua transform): Support metric events in version 2#2095
4 commits merged intomasterfrom
lua-v2-metrics

Conversation

@ghost
Copy link

@ghost ghost commented Mar 19, 2020

This PR adds support for both metrics and log events in lua support APIv2 in accordance to RFC 1999.

The change is not documented because Lua API v2 is not stable yet, so it is not advertised in the documentation.

Closes #1563, closes #1406, and closes #706 (for Lua APIv2).

Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>
@github-actions
Copy link

github-actions bot commented Mar 19, 2020

Great PR! Please pay attention to the following items before merging:

Files matching src/**:

  • For each failure path, is there sufficient context logged for users to investigate the issue?
  • Do the tests ensure that behavior is sane for inputs that don't meet normal assumptions (e.g. missing field, non-string, etc)?
  • Did you add adequate documentation?

Files matching src/transforms/**:

  • Did you add behavior tests (tests/behavior) that represent the behavior of this change? For example, testing for Vector's field path notation for nested fields.

This is an automatically generated QA checklist based on modified files

"type(log.nested) == 'table'",
"log.nested.field == '2'",
"#log.nested.array == 3",
"log.nested.array[1] == 'example value'",
Copy link
Contributor

@Hoverbear Hoverbear Mar 21, 2020

Choose a reason for hiding this comment

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

Oh no, I hate this. Array indexing is going to be my bane! Hahaha

Copy link
Author

Choose a reason for hiding this comment

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

In principle, it possible to make it zero-based, as Lua arrays are just hash tables with numeric indexes. But third-party Lua libraries might not handle this well.

count: aggregated_summary.get("count")?,
sum: aggregated_summary.get("sum")?,
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunate we can't make this a match statement to avoid this breaking accidentally in future metric type additions.

Copy link
Author

Choose a reason for hiding this comment

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

I considered devising a serde-style derive macro which would implement FromLua for arbitrary enums, but decided against in the initial implementation because I thought that it might add too much complexity, and also it is not clear what to do with DateTime<Utc> and Bytes because both of these types are external, so FromLua cannot be implemented for them and it has to be worked around somehow.

But doing this this could have solved the issue of possible breaking of this in case if new metric type is added.

@Hoverbear
Copy link
Contributor

Looks good. :) Had a suggestion to improve the error message, but otherwise free to merge.

@Hoverbear Hoverbear added domain: transforms Anything related to Vector's transform components transform: lua Anything `lua` transform related labels Mar 21, 2020
@binarylogic
Copy link
Contributor

From planning: Adding final touches before merging.

a-rodin added 3 commits March 23, 2020 21:48
Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>
Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>
Signed-off-by: Alexander Rodin <rodin.alexander@gmail.com>
@binarylogic binarylogic added the needs: docs Needs documentation updates label Mar 23, 2020
@binarylogic binarylogic self-requested a review March 23, 2020 19:27
@binarylogic
Copy link
Contributor

@a-rodin we'll need to update the docs before merging this. This will be a little different in that we'll want good examples and properly document the hooks API.

@ghost
Copy link
Author

ghost commented Mar 23, 2020

@binarylogic I'll add docs and examples for this.

But I have two questions:

  1. Where to put the documentation for version 2 of the transform? Should it be in website/docs/reference/transforms/lua.md.erb or in a separate file?
  2. Although this PR doesn't implement hooks, would it make sense to document all parts of RFC 1999 at once, and then just add implementations for them?

@binarylogic
Copy link
Contributor

Where to put the documentation for version 2 of the transform? Should it be in website/docs/reference/transforms/lua.md.erb or in a separate file?

In the same file, no need to create a new one. Take a look at the influxdb_metrics.toml file. Notice the use of groups.

Although this PR doesn't implement hooks, would it make sense to document all parts of RFC 1999 at once, and then just add implementations for them?

It's up to you. I want to discourage our docs from getting ahead of the code. If we want to do it all in one shot then I would merge this, implement the hooks, and then follow it all up with docs.

In general though, we should be encouraging users to use v2 and not v1.

@ghost
Copy link
Author

ghost commented Mar 24, 2020

I want to discourage our docs from getting ahead of the code. If we want to do it all in one shot then I would merge this

I would like to proceed this way: implement hooks (at least hooks.process and then add the documentation). The reason is that I want the documentation to display only stable APIs.

@ghost ghost merged commit ff4a6dc into master Mar 24, 2020
@ghost ghost deleted the lua-v2-metrics branch March 25, 2020 17:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: transforms Anything related to Vector's transform components needs: docs Needs documentation updates transform: lua Anything `lua` transform related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support metrics events in lua transform Nested fields and data types Restore event structure before passing it to the lua runtime

3 participants