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

receiver_creator: add dynamic configuration values #235

Merged
merged 5 commits into from
May 20, 2020
Merged

receiver_creator: add dynamic configuration values #235

merged 5 commits into from
May 20, 2020

Conversation

jrcamp
Copy link
Contributor

@jrcamp jrcamp commented May 14, 2020

This adds the ability to configure a receiver based on values from the
observer. This might be from labels or some other metadata the user sets. This
includes setting the port if the port is not able to be discovered through
standard means.

For example, in k8s it is optional for a pod to define the ports containers are
listening on. Users may need to set those ports inside the config. This is
possible with dynamic configuration.

Will add docs to readme after other PR is merged.

Example:

receivers:
  receiver_creator:
    redis:
      rule: type.pod && labels["app"] == "redis"
      config:
        endpoint: "`endpoint`:8382"

This adds the ability to configure a receiver based on values from the
observer. This might be from labels or some other metadata the user sets. This
includes setting the port if the port is not able to be discovered through
standard means.

For example, in k8s it is optional for a pod to define the ports containers are
listening on. Users may need to set those ports inside the config. This is
possible with dynamic configuration.

Will add docs to readme after other PR is merged.

Example:

```yaml
receivers:
  receiver_creator:
    redis:
      rule: type.pod && labels["app"] == "redis"
      config:
        endpoint: "`endpoint`:8382"
```

The text between backticks (in this case `endpoint`) is evaluated using expr
(same type of expressions as with discovery rules).
@jrcamp jrcamp requested a review from a team as a code owner May 14, 2020 22:59
@codecov-io
Copy link

codecov-io commented May 14, 2020

Codecov Report

Merging #235 into master will decrease coverage by 0.04%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   77.86%   77.81%   -0.05%     
==========================================
  Files         125      126       +1     
  Lines        6455     6528      +73     
==========================================
+ Hits         5026     5080      +54     
- Misses       1155     1167      +12     
- Partials      274      281       +7     
Impacted Files Coverage Δ
receiver/receivercreator/observerhandler.go 61.29% <58.33%> (-4.62%) ⬇️
receiver/receivercreator/config_expansion.go 84.31% <84.31%> (ø)
receiver/receivercreator/rules.go 84.09% <93.10%> (+1.59%) ⬆️
exporter/awsxrayexporter/translator/segment.go 89.05% <0.00%> (-1.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac13400...03ac850. Read the comment docs.

@asuresh4
Copy link
Member

LGTM

@tigrannajaryan tigrannajaryan added this to Review in progress in Collector May 19, 2020
"github.com/antonmedv/expr"
)

func expandConfigValue(env endpointEnv, configValue string) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment to explain what does this expand and how.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand the code correctly this evaluates expressions inside backticks and replaces them with the result of evaluation. The name expandConfigValue does not quite convey this. Perhaps evalBackticksInConfigValue?


// expandMap recursively expands any expressions in backticks and returns a copy
// of the resolved config.
func expandMap(cfg map[string]interface{}, env endpointEnv) (map[string]interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

what does env parameter do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment to include env.

// itself so that it retains the type returned by the expression. Might be a bool, int, etc.
// instead of a string.
if len(expansions) == 1 && output.String() == fmt.Sprintf("%v", expansions[0]) {
return expansions[0], nil
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand this exception and how it behaves differently from the case when there are more than one expansions. Maybe add an example in the comment to make it clear.
Why does this function need to retain the type? Does this mean that:

`true` evaluates to boolean true
` true` evaluates to string " true"

This may be a surprising behavior.

Copy link
Contributor Author

@jrcamp jrcamp May 20, 2020

Choose a reason for hiding this comment

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

Yeah if you say:

`true``false` it will return string "truefalse" as there's really no way to concat true and false booleans. Same with numbers.

However if you say ` true` it will evaluate to true boolean. The expression itself inside the backticks is not whitespace-sensitive. I've added a test case for it.

I'll also add some examples to the function doc.

An example where this matters might be:

config:
  secure: `"secure" in labels`

There might be some magic in Viper/mapstructure that might check for "true" and "false" strings when deserializing but I thought it best to keep the types if possible.

@tigrannajaryan tigrannajaryan self-assigned this May 20, 2020
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

Collector automation moved this from Review in progress to Reviewer approved May 20, 2020
@tigrannajaryan tigrannajaryan merged commit 95efa1b into open-telemetry:master May 20, 2020
Collector automation moved this from Reviewer approved to Done May 20, 2020
@jrcamp jrcamp deleted the dynamic-config branch May 20, 2020 18:17
wyTrivail referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 13, 2020
This adds the ability to configure a receiver based on values from the
observer. This might be from labels or some other metadata the user sets. This
includes setting the port if the port is not able to be discovered through
standard means.

For example, in k8s it is optional for a pod to define the ports containers are
listening on. Users may need to set those ports inside the config. This is
possible with dynamic configuration.

Will add docs to readme after other PR is merged.

Example:

```yaml
receivers:
  receiver_creator:
    redis:
      rule: type.pod && labels["app"] == "redis"
      config:
        endpoint: "`endpoint`:8382"
```

The text between backticks (in this case `endpoint`) is evaluated using expr
(same type of expressions as with discovery rules).
mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
- Exporters not used in any pipeline cause panic during shutting
  down.

Signed-off-by: Hui Kang <kangh@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants