chore: update path for local config in production readme #14259

LashaJini · 2024-09-25T12:38:13Z

What this PR does / why we need it:

👋, According to Build and run from source promtail should start without issues when only config file is passed (without passing any additional flags). However, because of the missing fields in the promtail-local-config.yaml file, it fails.

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

CLAassistant · 2024-09-25T12:38:25Z

All committers have signed the CLA.

LashaJini · 2024-09-25T12:39:27Z

clients/cmd/promtail/promtail-local-config.yaml

@@ -1,9 +1,13 @@
 server:
  http_listen_port: 9080
  grpc_listen_port: 0
+  log_level: debug


https://github.com/grafana/loki/blob/main/clients/cmd/promtail/main.go#L106

LashaJini · 2024-09-25T12:41:11Z

clients/cmd/promtail/promtail-local-config.yaml


 positions:
  filename: /tmp/positions.yaml
+  sync_period: 10s


https://github.com/grafana/loki/blob/main/clients/pkg/promtail/positions/positions.go#L154

LashaJini · 2024-09-25T12:42:22Z

clients/cmd/promtail/promtail-local-config.yaml


 positions:
  filename: /tmp/positions.yaml
+  sync_period: 10s
+target_config:
+  sync_period: 10s


https://github.com/grafana/loki/blob/main/clients/pkg/promtail/targets/file/filetarget.go#L210

trevorwhitney

Thanks for the contribution. Can you share the error you recieved? I was able to start up without problem using this config locally. Some of the configs you're setting we have defaults for, for example log level

LashaJini · 2024-10-03T08:45:10Z

Thanks for the contribution. Can you share the error you recieved? I was able to start up without problem using this config locally. Some of the configs you're setting we have defaults for, for example log level

with current config:

After passing the log.level flag:

According to docs, I just assumed that passing the config file only, should be sufficient to start promtail.

trevorwhitney · 2024-10-03T17:08:21Z

you're using two different config paths in that example. cmd/promtail/promtail-local-config.yaml in the first screenshot should be clients/cmd/promtail/promtail-local-config.yaml

LashaJini · 2024-10-03T18:05:02Z

you're using two different config paths in that example. cmd/promtail/promtail-local-config.yaml in the first screenshot should be clients/cmd/promtail/promtail-local-config.yaml

My bad, wrong screenshot. I've added the correct one 👍

There is no promtail config in cmd directory, even though the docs states so.

trevorwhitney · 2024-10-03T18:45:43Z

Got it, thanks for the update, I was able to re-create. Thanks for offering a fix, but I think we should fix the code instead of just adding a band-aid to the config file. I spent a little time looking at this and I'm not sure why the level config is empty when we check it. We are definitely calling .RegisterFlags on it, which sets the level to info, so this seems like a regression to me.

LashaJini · 2024-10-03T21:54:50Z

Let me know if we should close the PR and open a new one. Meanwhile I can also look into it later 👍

cstyan · 2024-10-04T16:40:18Z

Got it, thanks for the update, I was able to re-create. Thanks for offering a fix, but I think we should fix the code instead of just adding a band-aid to the config file. I spent a little time looking at this and I'm not sure why the level config is empty when we check it. We are definitely calling .RegisterFlags on it, which sets the level to info, so this seems like a regression to me.

I was thinking the same recently, I think maybe https://github.com/grafana/loki/pull/13719/files is the cause of this as it might have changed how default values for some config structs are applied.

LashaJini · 2024-10-04T17:24:15Z

Got it, thanks for the update, I was able to re-create. Thanks for offering a fix, but I think we should fix the code instead of just adding a band-aid to the config file. I spent a little time looking at this and I'm not sure why the level config is empty when we check it. We are definitely calling .RegisterFlags on it, which sets the level to info, so this seems like a regression to me.

I was thinking the same recently, I think maybe https://github.com/grafana/loki/pull/13719/files is the cause of this as it might have changed how default values for some config structs are applied.

Just tested it. Commented out the whole UnmarshalYAML method, rebuilt promtail and works without any modification to the original config file 👍. but why 😃

trevorwhitney · 2024-10-04T19:09:07Z

@fgouteroux looks like #13719 may have introduced a regression here, thoughts on a fix?

fgouteroux · 2024-10-07T08:49:09Z

@fgouteroux looks like #13719 may have introduced a regression here, thoughts on a fix?

I think its is caused by: https://github.com/grafana/loki/blob/main/clients/pkg/promtail/config/config.go#L45 this is why there is default config var in https://github.com/prometheus/prometheus/blob/main/config/config.go#L146.

That explain why any default value from flag are not kept after UnmarshalYAML func.

cstyan · 2024-10-07T17:03:34Z

@fgouteroux looks like #13719 may have introduced a regression here, thoughts on a fix?

I think its is caused by: https://github.com/grafana/loki/blob/main/clients/pkg/promtail/config/config.go#L45 this is why there is default config var in https://github.com/prometheus/prometheus/blob/main/config/config.go#L146.

That explain why any default value from flag are not kept after UnmarshalYAML func.

The second link you gave was to Prometheus code, so it's not relevant to the issue reported here. For Loki and Promtail we use default values via our CLI arguments, so there were default values that were being overwritten by the UnmarshalYAML function. #14408 fixes the proble, so this PR shouldn't be necessary anymore.

@LashaJini let us know if you still see any similar problems.

fgouteroux · 2024-10-07T17:07:19Z

oh I miss this line 😂

LashaJini · 2024-10-07T17:12:17Z

@fgouteroux looks like #13719 may have introduced a regression here, thoughts on a fix?

I think its is caused by: https://github.com/grafana/loki/blob/main/clients/pkg/promtail/config/config.go#L45 this is why there is default config var in https://github.com/prometheus/prometheus/blob/main/config/config.go#L146.
That explain why any default value from flag are not kept after UnmarshalYAML func.

The second link you gave was to Prometheus code, so it's not relevant to the issue reported here. For Loki and Promtail we use default values via our CLI arguments, so there were default values that were being overwritten by the UnmarshalYAML function. #14408 fixes the proble, so this PR shouldn't be necessary anymore.

@LashaJini let us know if you still see any similar problems.

Awesome. Then it's just a matter of doc update. We can merge now 👍

trevorwhitney

lgtm

pull-request-size bot added the size/XS label Sep 25, 2024

LashaJini commented Sep 25, 2024

View reviewed changes

LashaJini force-pushed the add-missing-fields-in-promtail-local-config branch 2 times, most recently from 9d04b38 to ea3cc5f Compare September 25, 2024 13:02

LashaJini marked this pull request as ready for review September 25, 2024 14:41

LashaJini requested a review from a team as a code owner September 25, 2024 14:42

trevorwhitney requested changes Sep 30, 2024

View reviewed changes

cstyan mentioned this pull request Oct 7, 2024

fix: promtail config unmarshalling #14408

Merged

LashaJini added 2 commits October 7, 2024 21:10

chore: Add missing fields in promtail local config

30e00df

chore: revert back config file

6aa401e

LashaJini force-pushed the add-missing-fields-in-promtail-local-config branch from ea3cc5f to 6aa401e Compare October 7, 2024 17:11

LashaJini requested a review from trevorwhitney October 7, 2024 17:13

cstyan approved these changes Oct 10, 2024

View reviewed changes

cstyan changed the title ~~chore: Fix promtail local config~~ chore: update path for local config in production readme Oct 18, 2024

trevorwhitney approved these changes Oct 18, 2024

View reviewed changes

trevorwhitney merged commit ab1cc52 into grafana:main Oct 18, 2024
60 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update path for local config in production readme #14259

chore: update path for local config in production readme #14259

LashaJini commented Sep 25, 2024 •

edited

Loading

CLAassistant commented Sep 25, 2024 •

edited

Loading

LashaJini Sep 25, 2024

LashaJini Sep 25, 2024

LashaJini Sep 25, 2024

trevorwhitney left a comment

LashaJini commented Oct 3, 2024 •

edited

Loading

trevorwhitney commented Oct 3, 2024

LashaJini commented Oct 3, 2024 •

edited

Loading

trevorwhitney commented Oct 3, 2024

LashaJini commented Oct 3, 2024

cstyan commented Oct 4, 2024

LashaJini commented Oct 4, 2024 •

edited

Loading

trevorwhitney commented Oct 4, 2024

fgouteroux commented Oct 7, 2024

cstyan commented Oct 7, 2024

fgouteroux commented Oct 7, 2024

LashaJini commented Oct 7, 2024

trevorwhitney left a comment

chore: update path for local config in production readme #14259

chore: update path for local config in production readme #14259

Conversation

LashaJini commented Sep 25, 2024 • edited Loading

CLAassistant commented Sep 25, 2024 • edited Loading

LashaJini Sep 25, 2024

Choose a reason for hiding this comment

LashaJini Sep 25, 2024

Choose a reason for hiding this comment

LashaJini Sep 25, 2024

Choose a reason for hiding this comment

trevorwhitney left a comment

Choose a reason for hiding this comment

LashaJini commented Oct 3, 2024 • edited Loading

trevorwhitney commented Oct 3, 2024

LashaJini commented Oct 3, 2024 • edited Loading

trevorwhitney commented Oct 3, 2024

LashaJini commented Oct 3, 2024

cstyan commented Oct 4, 2024

LashaJini commented Oct 4, 2024 • edited Loading

trevorwhitney commented Oct 4, 2024

fgouteroux commented Oct 7, 2024

cstyan commented Oct 7, 2024

fgouteroux commented Oct 7, 2024

LashaJini commented Oct 7, 2024

trevorwhitney left a comment

Choose a reason for hiding this comment

LashaJini commented Sep 25, 2024 •

edited

Loading

CLAassistant commented Sep 25, 2024 •

edited

Loading

LashaJini commented Oct 3, 2024 •

edited

Loading

LashaJini commented Oct 3, 2024 •

edited

Loading

LashaJini commented Oct 4, 2024 •

edited

Loading