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

[promtail helm chart] Enable support for syslog service #1617

Merged
merged 8 commits into from
Feb 14, 2020

Conversation

billimek
Copy link
Contributor

@billimek billimek commented Jan 31, 2020

What this PR does / why we need it:

This should allow the chart to be configured for an optional syslog service

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@claassistantio
Copy link

claassistantio commented Jan 31, 2020

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Jan 31, 2020

Codecov Report

Merging #1617 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1617   +/-   ##
=======================================
  Coverage   61.74%   61.74%           
=======================================
  Files         109      109           
  Lines        8308     8308           
=======================================
  Hits         5130     5130           
  Misses       2783     2783           
  Partials      395      395

@billimek billimek changed the title [promtail helm chart] Enable support for dynamic extra ports [promtail helm chart] Enable support for syslog service Feb 1, 2020
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 1, 2020
@cyriltovena cyriltovena requested review from rfratto and removed request for sandeepsukhani February 5, 2020 14:54
This should allow the chart to be configured for extra exposed ports
like syslog (port 1514) for example

Signed-off-by: Jeff Billimek <jeff@billimek.com>
Signed-off-by: Jeff Billimek <jeff@billimek.com>
Signed-off-by: Jeff Billimek <jeff@billimek.com>
Signed-off-by: Jeff Billimek <jeff@billimek.com>
@billimek
Copy link
Contributor Author

Hi @rfratto, is there anything I can do with this PR to help it move along?

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Hi @billimek, thank you for working on this! I'm not a Helm expert, so I'm afraid I can't give this the review it deserves. Just at a quick glance, my primary concern is that I don't see where it configures the syslog scrape config. Should that be included here?

@joe-elliott Could you take a look at this since you're more familiar with Helm?

@billimek
Copy link
Contributor Author

billimek commented Feb 11, 2020

Thanks @rfratto. As I understand it, the configuration of syslog would be handled in the extraScrapeConfigs section of the promtail helm chart configuration, e.g.

      extraScrapeConfigs:
      - job_name: syslog
        syslog:
          listen_address: 0.0.0.0:1514
          labels:
            job: "syslog"
        relabel_configs:
        - source_labels: ['__syslog_message_hostname']
          target_label: 'host'

What's missing is having the promtail helm chart actually expose a syslog endpoint via a service - which is what this PR is attempting to solve.

We could consider enhancing this pull request to encompass the entire syslog configuration. What do you think @joe-elliott?

@cyriltovena
Copy link
Contributor

Looks good @billimek can you improve the documentation of the helm chart on how to use the syslog in promtail ? With the snippet above ? here https://github.com/grafana/loki/tree/master/production/helm

Thanks !

billimek and others added 3 commits February 13, 2020 13:00
Signed-off-by: Jeff Billimek <jeffrey_k_billimek@homedepot.com>
* 'extraPorts' of github.com:billimek/loki: (25 commits)
  Ensure status codes are set correctly in the frontend. (grafana#1684)
  --dry-run Promtail. (grafana#1652)
  Fix logcli --quiet parameter parsing issue (grafana#1682)
  This improves the log ouput for statistics in the logcli. (grafana#1644)
  Loki stack helm chart can deploy datasources without Grafana (grafana#1688)
  Automatically prune metrics from the /metrics output of the promtail metrics pipeline stage after an idle period.
  Allow a metric defined as a counter to match all lines, useful for creating line count metrics which include all your labels. Found a bug in the the test runner where it didn't fail if the actual error was nil but the test expected an error Added some tests to the counters to test valid configs
  maintainer links & usernames (grafana#1675)
  Binary operators in LogQL (grafana#1662)
  Pipe data to Promtail (grafana#1649)
  Add Owen to the maintainer team. (grafana#1673)
  Update tanka.md so that promtail.yml is in the correct format (grafana#1671)
  Correcte syntax of rate example (grafana#1641)
  Frontend & Querier query statistics instrumentation. (grafana#1661)
  loki-canary: fix indent of DaemonSet manifest written in .md file (grafana#1648)
  Query frontend service should be headless. (grafana#1665)
  Support custom prefix name in metrics stage (grafana#1664)
  pkg/promtail/positions: handle empty positions file (grafana#1660)
  Convert second(Integer class) to nanosecond precision (grafana#1656)
  Unite docs for fluentd plugin (grafana#1634)
  ...
@billimek
Copy link
Contributor Author

Looks good @billimek can you improve the documentation of the helm chart on how to use the syslog in promtail ? With the snippet above ? here https://github.com/grafana/loki/tree/master/production/helm

Thanks @cyriltovena - this should be present now. Can you please take a look?

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

5 participants