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 Kafka target #4568

Merged
merged 22 commits into from
Nov 4, 2021
Merged

Promtail Kafka target #4568

merged 22 commits into from
Nov 4, 2021

Conversation

cyriltovena
Copy link
Contributor

What this PR does / why we need it:

This PR introduce Kafka support for Promtail. This is just a Draft PR because I'm still working on the documentation and testing.

But I still wanted to give more visibility to the team in the meantime.

Which issue(s) this PR fixes:
Fixes #4537

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Still needs to finish tests.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena marked this pull request as ready for review November 3, 2021 16:04
@@ -182,7 +182,7 @@ You can relabel default labels via [Relabeling](#relabeling) if required.
Providing a path to a bookmark is mandatory, it will be used to persist the last event processed and allow
resuming the target without skipping logs.

see the [configuration](./configuration.md#windows_events) section for more information.
see the [configuration](https://grafana.com/docs/loki/latest/clients/promtail/configuration/#windows_events) section for more information.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KMiller-Grafana Not sure how we should proceed with those link, but previously it would not work on the website.

We could use relref short code but this means it doesn't work on github since it's only supported for hugo and not markdown.

In the meantime I've replace it with an absolute link, which might not respect the version sadly.

WDYT ? I don't think we should block the review because of this though, let take care of it later if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, ./configuration.md#windows_events doesn't work? I wonder if it has to do with _ (using spaces creates links with -). Anyways, it's definitely preferable to use the local links as it allows the site generation tool to create entire sections of the docs for different releases and not always point to latest.

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.

Admittedly, I skimmed a lot of this, but overall it looks great. I've left a few comments, but am approving. Nice work!

Comment on lines 193 to 196
"__topic": model.LabelValue(claim.Topic()),
"__partition": model.LabelValue(fmt.Sprintf("%d", claim.Partition())),
"__member_id": model.LabelValue(session.MemberID()),
"__group_id": model.LabelValue(ts.cfg.KafkaConfig.GroupID),
Copy link
Member

Choose a reason for hiding this comment

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

Should these be further prefixed with __meta_kafka, like we have for __meta_kubernetes_<etc>?

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 good idea

docs/sources/clients/promtail/configuration.md Outdated Show resolved Hide resolved
@@ -182,7 +182,7 @@ You can relabel default labels via [Relabeling](#relabeling) if required.
Providing a path to a bookmark is mandatory, it will be used to persist the last event processed and allow
resuming the target without skipping logs.

see the [configuration](./configuration.md#windows_events) section for more information.
see the [configuration](https://grafana.com/docs/loki/latest/clients/promtail/configuration/#windows_events) section for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, ./configuration.md#windows_events doesn't work? I wonder if it has to do with _ (using spaces creates links with -). Anyways, it's definitely preferable to use the local links as it allows the site generation tool to create entire sections of the docs for different releases and not always point to latest.

slim-bean and others added 3 commits November 3, 2021 16:12
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena merged commit c8f3df3 into main Nov 4, 2021
@cyriltovena cyriltovena deleted the kafka branch November 4, 2021 11:17
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.

Add Kafka promtail target
3 participants