app/vlagent/remotewrite: introduce VictoriaLogs transformations language#1508
app/vlagent/remotewrite: introduce VictoriaLogs transformations language#1508vadimalekseev wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
3 issues found across 33 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
1e493dd to
e300516
Compare
39f6deb to
c3556c4
Compare
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| fields := rows[0] | ||
| for i := range fields { | ||
| name := br.addValue(fields[i].Name) | ||
| name := getCanonicalColumnName(fields[i].Name) |
There was a problem hiding this comment.
These changes are not related to log transformations, but without them, my tests fail.
8ad3249 to
e286231
Compare
| // | ||
| // It stops calling visitFunc on the remaining filters as soon as visitFunc returns true. | ||
| // It returns the result of the last visitFunc call. | ||
| func visitFilterRecursiveForPipe(p pipe, visitFunc func(f filter) bool) bool { |
There was a problem hiding this comment.
This code was written in the hope that it would be replaced with supporting relative time filters in VictoriaLogs transformations.
fa0bd2b to
2e51424
Compare
| timestamps = br.getTimestamps() | ||
| } else { | ||
| // Slow path: the _time column was overwritten or removed, initialize it again. | ||
| br.initTimestamps() |
There was a problem hiding this comment.
Inside initTimestamps, if brSrc is not nil, it takes the timestamps from brSrc. So if our blockResult has an overwritten _time column, that column is ignored:
VictoriaLogs/lib/logstorage/block_result.go
Lines 632 to 636 in 2e51424
In an if branch, when the filter matches only part of the block, a new blockResult is created with the brSrc set:
VictoriaLogs/lib/logstorage/transforms.go
Lines 558 to 559 in 2e51424
VictoriaLogs/lib/logstorage/block_result.go
Lines 141 to 149 in 1796949
Combining both, with a filter like:
if (service:=payment) {
time_add 1h;
}
on this data:
{"_time":"2026-07-01T10:00:00Z","service":"products","_msg":"a"}
{"_time":"2026-07-01T10:05:00Z","service":"products","_msg":"b"}
{"_time":"2026-07-01T10:00:00Z","service":"payment","_msg":"c"}
{"_time":"2026-07-01T10:05:00Z","service":"payment","_msg":"d"}
The payment rows keep their original _time instead of the one time_add produced
There was a problem hiding this comment.
Thank you! Fixed.
br.getTimestamps now initializes timestamps if timestampBuf is empty (original behavior). Additionally, it parses the _time column if timestampBuf is not empty (initialized) but contains outdated values (isTime == false).
It looks like either I am misusing getTimestamps(), or the issue is also present in the original code. Could you please validate the new behavior?
There was a problem hiding this comment.
I checked the other callers of getTimestamps() - they all check c.isTime first before calling it. So getTimestamps() is only called when _time still holds the real timestamps. That is why this problem does not happen elsewhere.
About the new fix: time_add works now, but format / copy / rename ... as _time still do not. time_add fills timestampsBuf first to read time, so it's not empty. But other pipes only create new _time columns without reading _time first, so timestampsBuf is empty in these cases.
One solution to fix this could be to change initFromBlockResult. Maybe we only call br.getTimestamps() if the column _time is still isTime, or else we parse it? I am not very sure this is best in term of performance, but it will be correct since it just copies from how other pipes do it before calling getTimestamps().
2f55dd2 to
4bba6e3
Compare
366ec73 to
b3ffaeb
Compare
f05c913 to
d940917
Compare
| putColumnIdxs(columnIdxs) | ||
| } | ||
|
|
||
| func (br *blockResult) mustInitFromLogRows(lr *LogRows) { |
There was a problem hiding this comment.
Minor, not a change request: this name reads like a generic LogRows→blockResult converter, but it actually contains transform-specific logic (vl_account_id / vl_project_id, and it's only called from the transformer). This might cause a little confusion or misuse later. Same for initFromBlockResult. Feel free to resolve, just flagging it.
| so the target tenant is controlled through Kubernetes annotations. | ||
|
|
||
| If `vl_account_id` and `vl_project_id` fields arrive in the incoming log, | ||
| `vlagent` treats them as ordinary fields and does not change the tenant. |
There was a problem hiding this comment.
treats them as ordinary fields
The docs might be wrong here. From the code in initFromBlockResult, these fields always stripped from the result.
There was a problem hiding this comment.
The documentation is correct, as mustInitFromLogRows overrides user-defined vl_account_id and vl_project_id fields:
- We call
addResultColumnhere:VictoriaLogs/lib/logstorage/block_result.go
Line 377 in d940917
addResultColumnreplaces existing column:VictoriaLogs/lib/logstorage/block_result.go
Line 2279 in d940917
This behavior is intentional: if a user enabled log transformations and passed vl_account_id in the request, but later disabled transformations, the logs would be written to the default tenant (because we won't call the initFromBlockResult function). The current approach is more explicit: if you want to set the tenant dynamically, you must provide it and set up an explicit transformation.
Documentation and syntax: https://github.com/VictoriaMetrics/VictoriaLogs/blob/vlt/docs/victorialogs/vlagent/transformations.md
Closes #858