-
Notifications
You must be signed in to change notification settings - Fork 270
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
feat: Add new stdlib function to_json #2860
base: main
Are you sure you want to change the base?
Conversation
@@ -61,6 +61,22 @@ The `encoding.to_base64` function encodes the original string into RFC4648-compl | |||
c3RyaW5nMTIzIT8kKiYoKSctPUB- | |||
``` | |||
|
|||
## encoding.to_json | |||
|
|||
The `encoding.to_json` function encodes a map into a JSON. |
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.
Thanks for your contribution, the code looks good but it looks like it can handle other types that map. Could you add tests for the different types and document the ones that don't work? (Not sure what happens if you try to encode a Secret or a Capsule for example)
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 have updated the tests and documentation. For Secret and Capsule the function converts the public fields of the underlying struct. I updated the doc with the closest understandable text. Should we handle this differently ?
Example configuration the I tried for verifying the behaviour,
discovery.relabel "keep_backend_only" {
targets = [
{ "__meta_foo" = "foo", "__address__" = "localhost", "instance" = "ones", "app" = "backend" },
{ "__meta_bar" = "bar", "__address__" = "localhost", "instance" = "two", "app" = "database" },
{ "__meta_baz" = "baz", "__address__" = "localhost", "instance" = "three", "app" = "frontend" },
]
rule {
source_labels = ["__address__", "instance"]
separator = "/"
target_label = "destination"
action = "replace"
}
rule {
source_labels = ["app"]
action = "keep"
regex = "backend"
}
}
declare "discovery_result" {
argument "output_value" {
optional = false
}
argument "rules_value" {
optional = false
}
argument "number_value" {
optional = true
default = 14
}
argument "array_value" {
optional = true
}
argument "string_value" {
optional = true
}
argument "null_value" {
optional = true
}
}
discovery_result "backend" {
output_value = encoding.to_json(discovery.relabel.keep_backend_only.output)
rules_value = encoding.to_json(discovery.relabel.keep_backend_only.rules)
number_value = encoding.to_json(23)
array_value = encoding.to_json([1,2,3])
string_value = encoding.to_json("Hello World")
null_value = encoding.to_json(null)
}
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.
Thanks for testing it, I think that the best approach would be to prevent capsule and secret/optional secrets from being converted because the only way to expose a secret should be through the nonsensitive function + I don't see a good usage to get the public fields exported of the capsule for now.
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 made the changes to support only map type instead of complicating the support for all other types.
The way i see it,
- we can add conditions on top of map[string]interface{} for other types int, float, string etc,
- We can have a recursive logic to convert the applicable types types since a map can have have nested maps and arrays can have map nested.
Let me know your thoughts or if we have any other approaches here ?
`encoding.to_json` fails if the map argument provided can't be parsed as json string. | ||
|
||
A common use case of `encoding.to_json` is to encode a config of component which is expected to be a JSON | ||
For example, `config` of a [`prometheus.exporter.blackbox`][] |
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.
For example, `config` of a [`prometheus.exporter.blackbox`][] | |
For example, configuration of a [`prometheus.exporter.blackbox`][] |
Missing a word here... a prometheus.exporter.blackbox
string?
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 wanted to refer the param name in the component updated accordingly.
a6e6e48
to
7ce2113
Compare
Fix documentation remove test
7ce2113
to
7189840
Compare
PR Description
Adds a new stdlib function for json encoding
to_json
which converts a map to JSONWhich issue(s) this PR fixes
Fixes #152
Notes to the Reviewer
PR Checklist