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 documentation: changing the headers of the configuration docu to reflect configuration code #2636

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

LTek-online
Copy link
Contributor

What this PR does / why we need it:

Greetings,

The configuration documentation of promtails contains some errors where the headers of the sections don't match the actual config.
Notably configuration keys like 'server' and 'clients' have been mislabeled as 'server_config' and 'client_config'.
This pull request is aimed to correct that so that it matches the configuration as in the written config yaml files to avoid confusion.

I would propose to change the following sections to the following new names:

Old name New name (same as in code)
server_config server
client_config clients
position_config positions
scrape_config scrape_configs
journal_config journal
syslog_config syslog
loki_push_api_config loki_push_api
relabel_config relabel_configs
static_config static_configs

Thanks

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

The configuration documentation of promtails contains some errors where the headers of the sections don't match the actual configs.
Notably configuration keys like 'server' and 'clients' have been mislabeled as 'server_config' and 'client_config'.
This merge request is aimed to correct that so that it matches the configuration as in the written config files to avoid confusion.
@CLAassistant
Copy link

CLAassistant commented Sep 17, 2020

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

Codecov Report

Merging #2636 into master will decrease coverage by 1.42%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2636      +/-   ##
==========================================
- Coverage   62.87%   61.45%   -1.43%     
==========================================
  Files         170      170              
  Lines       15051    13172    -1879     
==========================================
- Hits         9464     8095    -1369     
+ Misses       4827     4336     -491     
+ Partials      760      741      -19     
Impacted Files Coverage Δ
pkg/logql/step_evaluator.go 57.14% <0.00%> (-9.53%) ⬇️
pkg/logql/marshal/labels.go 66.66% <0.00%> (-8.34%) ⬇️
pkg/logproto/timestamp.go 40.00% <0.00%> (-6.81%) ⬇️
pkg/logentry/stages/labeldrop.go 53.33% <0.00%> (-6.67%) ⬇️
pkg/chunkenc/encoding_helpers.go 59.25% <0.00%> (-6.37%) ⬇️
pkg/logql/marshal/tail.go 66.66% <0.00%> (-6.07%) ⬇️
pkg/promtail/api/types.go 14.28% <0.00%> (-5.72%) ⬇️
pkg/logql/vector.go 81.81% <0.00%> (-5.69%) ⬇️
pkg/logql/sharding.go 53.89% <0.00%> (-5.60%) ⬇️
pkg/storage/stores/shipper/table_client.go 36.36% <0.00%> (-5.58%) ⬇️
... and 160 more

@slim-bean
Copy link
Collaborator

hey @LTek-online thanks for the PR! I think these names were intentional although I have found myself confused at times too. I will look at this a little closer tomorrow or early next week, but I think this might have been done this way to be consistent with cortex and prometheus documentation.

@stale
Copy link

stale bot commented Oct 18, 2020

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Oct 18, 2020
@LTek-online
Copy link
Contributor Author

Hi, could someone please look over this merge request?
Thanks.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Oct 19, 2020
@stale
Copy link

stale bot commented Nov 19, 2020

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Nov 19, 2020
@oddlittlebird
Copy link
Contributor

Go away, stalebot! We're working here.

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Nov 23, 2020
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

LGTM, does this need review from a developer as well?

@cyriltovena cyriltovena added the keepalive An issue or PR that will be kept alive and never marked as stale. label Nov 24, 2020
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Apologies for taking so long to review this. I really appreciate the simplification here. Many of these names referenced their internal source code names before, but I think documentation should use the naming conventions users will see, which you've done.

@slim-bean
Copy link
Collaborator

Yeah let's merge this, regardless of the initial reason I much prefer having the sections match the actual config object names.

@LTek-online would you be willing to do this same review for the Loki config documentation? ❤️

@owen-d owen-d merged commit 56830bc into grafana:master Nov 24, 2020
@LTek-online LTek-online deleted the patch-1 branch November 24, 2020 22:15
@LTek-online
Copy link
Contributor Author

Thanks, for approving.
I am currently pretty busy, so maybe I can review it in the future.
Going to open a new merge request if I notice it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive An issue or PR that will be kept alive and never marked as stale. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants