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

Expose http sources for metrics in values.yaml #672

Merged
merged 24 commits into from
Jun 1, 2020

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented May 21, 2020

Description

Expose http sources in values.yaml

ToDO
  • Move out configuration from configmap to separate files (like fluentd conf)
  • Discuss if logs/events/default metrics should be exposed
    I decided to not expose logs and events so far
  • Add enable/disable flag
  • Dynamic generation of fluentd configuration(?)
Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@sumo-drosiek sumo-drosiek marked this pull request as draft May 21, 2020 12:23
@sumo-drosiek sumo-drosiek added this to the v1.1 milestone May 21, 2020
@sumo-drosiek sumo-drosiek force-pushed the drosiek-dynamic-metric-sources branch 6 times, most recently from edcb57d to 57d0d60 Compare May 25, 2020 12:47
@sumo-drosiek sumo-drosiek changed the title [WIP] Expose http sources in values.yaml Expose http sources for metrics in values.yaml May 25, 2020
@sumo-drosiek sumo-drosiek marked this pull request as ready for review May 25, 2020 14:22
@sumo-drosiek sumo-drosiek force-pushed the drosiek-dynamic-metric-sources branch from 6aab75c to fbeab9e Compare May 26, 2020 09:23
Copy link
Contributor

@perk-sumo perk-sumo left a comment

Choose a reason for hiding this comment

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

Overall I like this but as discussed some UX changes are needed.

@sumo-drosiek sumo-drosiek force-pushed the drosiek-dynamic-metric-sources branch from 5968a97 to 0a6ceb3 Compare May 26, 2020 16:47
@sumo-drosiek
Copy link
Contributor Author

I updated the PR with more user-friendly configuration, but this is highly breaking change for non-helm users

@sumo-drosiek sumo-drosiek force-pushed the drosiek-dynamic-metric-sources branch 2 times, most recently from baa1727 to bab6432 Compare May 27, 2020 11:11
@sumo-drosiek
Copy link
Contributor Author

sumo-drosiek commented May 27, 2020

Here is example of adding new source to sumologic and fluentd:

    --set sumologic.sources.new_source.value="my new source" \ # create new source named my new source and available in helm as new_source
    --set fluentd.metrics.output.new_source.match="test_match" \ # fluentd <match test_match> clausule
    --set fluentd.metrics.output.new_source.id="test_id" \ # fluentd @id parameter
    --set fluentd.metrics.output.new_source.weight=0 \ # optional, default is 0, order of fluentd match (lower, earlier)
    --set fluentd.metrics.output.new_source.drop=true # optional, default is false, if true, drop traffic instead of passing it to the sumologic \
    --set fluentd.buffer.filePaths.metrics.new_source=/fluentd/buffer/metrics.apiserver # set persistence

@sumo-drosiek
Copy link
Contributor Author

sumo-drosiek commented May 27, 2020

Unfortunately if source value is changed, the new source is created via terraform and old one is not cleaned up

I suppose persistence setup job could fix it

writeRelabelConfigs:
- action: keep
regex: coredns
sourceLabels: [job]
- # etcd server
url: http://$(CHART).$(NAMESPACE).svc.cluster.local:9888/prometheus.metrics.control-plane
url: http://$(CHART).$(NAMESPACE).svc.cluster.local:9888/prometheus.metrics.control-plane.kube-etcd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes prometheus issue. It has been saying that configuration for http://$(CHART).$(NAMESPACE).svc.cluster.local:9888/prometheus.metrics.control-plane is duplicated

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing i find misleading with this setup is that we collect from other control plane components and do not prefix them with control-plane. Any reason to just remove the control-plane part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the fluentd configuration will be more complex: either more complex regex for tags or output section per endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

How many metrics is each of these producing? We need to be conscious of blacklisting and if we are close to the limit, we should distribute across multiiple HTTP sources to prevent that.

@vsinghal13 vsinghal13 self-requested a review June 1, 2020 16:14
@sumo-drosiek sumo-drosiek force-pushed the drosiek-dynamic-metric-sources branch from f96dadf to 3e52c14 Compare June 1, 2020 16:24
@sumo-drosiek sumo-drosiek merged commit cf96b8a into master Jun 1, 2020
@sumo-drosiek sumo-drosiek deleted the drosiek-dynamic-metric-sources branch June 1, 2020 16:38
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.

5 participants