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.

Sign up for GitHub

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

Jump to bottom

Part I: converting promtail configs to flow mode #4160

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Part I: converting promtail configs to flow mode #4160

merged 1 commit into from
Jul 14, 2023

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented Jun 14, 2023 •
edited
Loading

PR Description

This sets up foundations for the promtail configuration conversion to flow mode configuration.

There is some work remaining, but to limit the size of the PR, I am implementing only some features. Specifically, what is remaining is:

  • More SD components
  • More log source components
  • More pipeline stages support for loki.process

Notes to the Reviewer

There are two main kinds of pipelines that this supports:

  • Logs source based - see source_pipeline_example.river test - for example: loki.source.journal -> loki.process -> loki.write with relabel rules directly passed to loki.source.journal.
    • Some components will not support taking relabel rules directly, such as loki.source.cloudflare, so there is an extra loki.relabel added. See cloudflare_relabel.river test.
  • SD-based - see sd_pipeline_example.river test - for example: discovery.kubernetes -> discovery.relabel -> discovery.file -> loki.source.file -> loki.process -> loki.write.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch from 258a183 to e294abc Compare June 28, 2023 15:16
@thampiotr thampiotr changed the title WIP, please do not review WIP, please do not review - converting promtail configs to river Jun 28, 2023
@thampiotr thampiotr mentioned this pull request Jun 29, 2023
Merged
3 tasks
@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch 2 times, most recently from e483a87 to ee57df7 Compare July 4, 2023 09:17
@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch 6 times, most recently from 2821fca to 784d3f7 Compare July 10, 2023 13:46
@thampiotr thampiotr changed the title WIP, please do not review - converting promtail configs to river Part I: converting promtail configs to flow mode Jul 10, 2023
@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch from 2a21da0 to 03a5e6a Compare July 10, 2023 13:50
@thampiotr thampiotr marked this pull request as ready for review July 10, 2023 13:58
@thampiotr thampiotr requested a review from a team as a code owner July 10, 2023 13:58
@thampiotr thampiotr requested a review from rfratto July 10, 2023 14:03
rfratto
rfratto reviewed Jul 10, 2023
View reviewed changes
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.

I looked at this from the output of the conversion; mostly looks good! I only have one real question with a few small nits.

converter/internal/promtailconvert/testdata/cloudflare_relabel.river Show resolved Hide resolved
converter/internal/promtailconvert/testdata/journal.river
loki.source.journal "fun" {
format_as_json = true
max_age = "20h0m0s"
relabel_rules = null
Copy link
Member

Choose a reason for hiding this comment

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

It's a little surprising that this is getting written out 🤔

converter/internal/promtailconvert/testdata/kubernetes.river Outdated Show resolved Hide resolved
converter/internal/promtailconvert/testdata/remote_write.yaml Show resolved Hide resolved
converter/internal/promtailconvert/testdata/source_pipeline_example.river Outdated Show resolved Hide resolved
@erikbaranowski erikbaranowski self-requested a review July 11, 2023 19:23
erikbaranowski
erikbaranowski reviewed Jul 11, 2023
View reviewed changes
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

Great start to this new converter!

Heads up, I did modify how the prometheus validation is organized so your changes to prometheus converter code may cause a little merge weirdness. Hopefully it's not too bad since you were mostly just making some functionality public.

converter/internal/common/common.go Outdated Show resolved Hide resolved
converter/internal/common/common.go Show resolved Hide resolved
converter/internal/common/convert_appendable.go
@@ -19,7 +19,7 @@ type ConvertAppendable struct {

var _ storage.Appendable = (*ConvertAppendable)(nil)
var _ builder.Tokenizer = ConvertAppendable{}
var _ river.Capsule = ConvertTargets{}
var _ river.Capsule = ConvertAppendable{}
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for catching ❤️

converter/internal/promtailconvert/promtailconvert.go Outdated Show resolved Hide resolved
converter/internal/promtailconvert/promtailconvert.go Outdated Show resolved Hide resolved
converter/internal/promtailconvert/promtailconvert_test.go Show resolved Hide resolved
converter/internal/promtailconvert/testdata/remote_write.yaml Show resolved Hide resolved
converter/internal/promtailconvert/testdata/kubernetes.river Outdated Show resolved Hide resolved
converter/internal/promtailconvert/promtailconvert.go Show resolved Hide resolved
@thampiotr
Copy link
Contributor Author

Heads up, I did modify how the prometheus validation is organized so your changes to prometheus converter code may cause a little merge weirdness. Hopefully it's not too bad since you were mostly just making some functionality public.

Yes, I'm aware - that's the reason I didn't move the exported methods to the top of the file as one would typically do - I wanted to reduce merge issues. Feel free to refactor as you see fit though :)

@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch 2 times, most recently from 328d93b to 2fe4f2e Compare July 12, 2023 14:24
erikbaranowski
erikbaranowski approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

LGTM, great start!

@thampiotr thampiotr force-pushed the thampiotr/promtail-cfg-conversion branch from 2fe4f2e to 7fa2d8f Compare July 14, 2023 13:08
@erikbaranowski erikbaranowski merged commit 322ffd1 into main Jul 14, 2023
@erikbaranowski erikbaranowski deleted the thampiotr/promtail-cfg-conversion branch July 14, 2023 15:03
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
@thampiotr @clayton-cornell
promtail confing conversion (#4160)
2fe03af
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
@thampiotr @clayton-cornell
promtail confing conversion (#4160)
eab9427
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikbaranowski erikbaranowski erikbaranowski approved these changes

@rfratto rfratto Awaiting requested review from rfratto

Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thampiotr @rfratto @erikbaranowski