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

feat: Add new stdlib function to_json #2860

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ravishankar15
Copy link
Contributor

PR Description

Adds a new stdlib function for json encoding to_json which converts a map to JSON

Which issue(s) this PR fixes

Fixes #152

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@@ -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.
Copy link
Contributor

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)

Copy link
Contributor Author

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)
}

Copy link
Contributor

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.

Copy link
Contributor Author

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,

  1. we can add conditions on top of map[string]interface{} for other types int, float, string etc,
  2. 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`][]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

syntax: Add json_encode to stdlib
3 participants