-
Notifications
You must be signed in to change notification settings - Fork 487
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
Flow: add loki.source.azure_event_hubs component #3412
Conversation
eeb9da8
to
bd6a9f9
Compare
❤️ Thanks for the PR! We'll review this as soon as we can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! 🙌 Comments are mostly around documentation and style, this is looking good!
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
case <-ctx.Done(): | ||
return nil | ||
case entry := <-c.handler: | ||
c.mut.RLock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for @tpaschalis: we might be susceptible to something like #3391 here, both for this and the loki.source.kafka component.
Will check it out to see if there's a need for a fix after this is merged.
t, err := kt.NewSyncer(c.opts.Registerer, c.opts.Logger, cfg, entryHandler, &parser.AzureEventHubsTargetMessageParser{ | ||
DisallowCustomMessages: newArgs.DisallowCustomMessages, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other reviewers, here's the crucial bit: the shared kafkatarget
package was updated in Promtail to expect a Parser implementation, and they've added one for Azure Event Hubs.
9ce3acf
to
55c5406
Compare
Thank you for the review @tpaschalis! I've updated the PR based on your feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Nice! I reviewed this purely from the documentation perspective, and I'll let @tpaschalis sign off on the code side.
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
docs/sources/flow/reference/components/loki.source.azure_event_hubs.md
Outdated
Show resolved
Hide resolved
Thank you! Updated the changes now :) |
Remove non-recommended additional header as pointed out here: #3412 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @tpaschalis I'll leave final approval / merging to you :)
Remove non-recommended additional header as pointed out here: #3412 (comment)
Remove non-recommended additional header as pointed out here: #3412 (comment) (cherry picked from commit e2c8f54)
Remove non-recommended additional header as pointed out here: #3412 (comment) (cherry picked from commit e2c8f54) Co-authored-by: Robert Fratto <[email protected]>
7169060
to
809b6ca
Compare
@akselleirv apologies for the hassle, but we recently got a PR that mass updates files for typos, grammatical errors etc. Could you please rebase, and I think we're good to go! |
Co-authored-by: Paschalis Tsilias <[email protected]>
Co-authored-by: Paschalis Tsilias <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
Co-authored-by: Robert Fratto <[email protected]>
809b6ca
to
f53652d
Compare
@tpaschalis rebased now and the CI is all green 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you Aksel!
I solved the problem. The problem was loki api address is not 'url = "loki:3100/api/v1/push" in the docs (https://grafana.com/docs/agent/latest/flow/reference/components/loki.source.azure_event_hubs/#example). It should be "loki:3100/loki/api/v1/push" accroding to the docs(https://grafana.com/docs/loki/latest/api/) |
Remove non-recommended additional header as pointed out here: #3412 (comment)
Remove non-recommended additional header as pointed out here: #3412 (comment)
PR Description
Add a new component
loki.source.azure_event_hubs
which read messages from Azure Event Hub using Kafka and forwards them to other loki components. It is a port of this PR: grafana/loki#8787 with the addition of OAUTHBEARER authentication.The main motivation behind this PR is adding support for collecting logs from Azure resources.
This authentication mechanism is also added to the
loki.source.kafka
component as this component depends on it.Which issue(s) this PR fixes
Not related to an issue, but was created based on this conversation: https://grafana.slack.com/archives/C01050C3D8F/p1680105638704069
Notes to the Reviewer
PR Checklist