-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
enhancement(vrl): add encode_key_value
function
#7751
Conversation
57c4c10
to
439c059
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.
Just one comment on the documentation side of things, otherwise looks good to me
I wonder if that failed test is flakey 🤔 |
I think so as well, I believe that's not the first time it fails with no apparent reason or no obvious link to the code changed by the PR |
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 left a few initial comments.
There are two "higher-level" thoughts that I have:
-
Do we want to specifically tie this to Datadog, or is this pattern (tags using
foo:bar,baz:qux
) generic enough that we can give it a more generic name? -
If we do want to tie it to Datadog specifically, should we name it
datadog_add_tags
anddatadog_remove_tags
, similar to how we name all provider-specific parsing functionsparse_<provider>_...
(such asparse_aws_...
)?
I think you made a point about being generic, we could assume that a tag list is generic enough (add key/value delimiter and and tags delimiter as an optional field) and instead have function named |
Just noting that it seems similar to append with more structure |
There's also parse_key_value which specifically allows specifying It appears this is identical to using $ tags = "foo:bar,baz:qux"
"foo:bar,baz:qux"
$ parsed_tags = parse_key_value!(tags, field_delimiter: ",", key_value_delimiter: ":")
{ "baz": "qux", "foo": "bar" }
$ del(parsed_tags.baz)
"qux"
$ parsed_tags.quux = "quuz"
"quuz"
$ parsed_tags
{ "foo": "bar", "quux": "quuz" } Now, the thing that seems to be missing in VRL right now is "how to go from a k/v object to a k/v string?" We have convert functions for this. It seems to me what we really want in this case is $ .ddtags = to_key_value(parsed_tags, field_delimiter: ",", key_value_delimiter: ":")
"foo:bar,quux:quuz" Having said all this, I'm not against providing functions that make the above list of operations more convenient to perform, I'm just not entirely convinced the I'm happy to defer to others such as @jszwedko and @binarylogic to make that call. |
I was unaware of the
Bouncing on that edit: I reworked the PR accordingly |
9264e6c
to
bfbf21f
Compare
One difference I'm noticing with
We could modify
If I remember correctly, we also allow duplicate tags in Datadog which would throw another wrinkle into this. For example, what would I agree that these functions probably don't necessitate something Datadog specific but it also seems like That being said, I think this PR to add |
It seems a suggestion to handle standalone key already exists out there https://pkg.go.dev/github.com/kr/logfmt?utm_source=godoc Standalone key would end up being deserialised along with a As supporting standalone key (either as true or null for the value) seems doable without breaking any existing behaviour and also cover corner cases for datadog tags manipulation. Edit: the latest commit support standalone key and should not break anything (maybe the |
b57cf6b
to
ec7e590
Compare
I split the PR in 2 different PR :
|
encode_key_value
function
I just checked, it works without any error, however the former is dropped silently and the latter is returned, a way to support same key tags would be to extend array processing function, a new function not really tied to datadog tag manipulation per se is available in #7908 could help doing some tag filtering just using |
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, thanks @prognant ! This looks good to me for adding the encode_key_value
. I agree we could add an option to output keys with true
values as just the key as a follow-up. This might be required for the datadog tags work?
Could we add this new function to the benches?
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.
Sweet! Almost there.
want: Ok("agent.id=1234 agent.name=vector event=log log.file.path=encode_logfmt.rs network.ip.0=127 network.ip.1=0 network.ip.2=0 network.ip.3=1 network.proto=tcp"), | ||
tdef: TypeDef::new().bytes().fallible(), |
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.
Do we want to keep these (logfmt specific) tests, or were they moved to the new function?
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 did the same as parse_logfmt/parse_key_value, i.e. no tests in the encode_logfmt (Existing ones were relocated in encode_key_value). I find it consistent, and as default separators match the logfmt format it is not illogical.
} | ||
|
||
fn type_def(&self, _state: &state::Compiler) -> TypeDef { | ||
TypeDef::new().bytes().fallible() |
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.
It seems like we can make this function infallible if the fields_ordering
argument is omitted?
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 think so as well, I'll give it a try.
Indeed, I was planning to work on that after getting that one merged, I think it makes sense to have |
6840c7e
to
5628523
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.
Nice!
encode_key_value
functionencode_key_value
function
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.
Two minor remarks, but otherwise this LGTM
9cfe5cd
to
18688c0
Compare
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Co-authored-by: Jean Mertz <git@jeanmertz.com> Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
18688c0
to
8ccedb7
Compare
It reuses
encode_logfmt
and allows custom field & k/v delimiters.encode_logfmt
in now an alias with preset delimiters.It can be use to construct datadog log tags list: