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

Implement generic forwarder #1775

Merged
merged 12 commits into from
Oct 19, 2022
Merged

Implement generic forwarder #1775

merged 12 commits into from
Oct 19, 2022

Conversation

Blinkuu
Copy link
Contributor

@Blinkuu Blinkuu commented Oct 3, 2022 •
edited
Loading

What this PR does:

This PR introduces a new subcomponent to distributor named forwarder. This generic forwarder serves as a facade for forwarding batches of spans in "fire-and-forget" way. The feature is designed in such a way that it's configurable on per-tenant basis.

Example configuration:

# config.yaml
distributor:
  forwarders:
    - name: "otel-forwarder"
      backend: "otlpgrpc"
      otlpgrpc:
        endpoints: ['otelcol:4317']
        tls:
          insecure: true

# overrides.yaml
overrides:
  "example-tenant-1":
    forwarders: ['otel-forwarder']
  "example-tenant-2":
    forwarders: ['otel-forwarder']

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Blinkuu Blinkuu changed the title Implement generic forwarder WIP: Implement generic forwarder Oct 3, 2022
@Blinkuu Blinkuu self-assigned this Oct 5, 2022
@Blinkuu Blinkuu added the enhancement New feature or request label Oct 5, 2022
@Blinkuu Blinkuu marked this pull request as ready for review October 6, 2022 13:39
mapno
mapno reviewed Oct 7, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Gave it a quick review. I will come back to it later, it's a big PR. Overall I like the direction of the PR.

modules/distributor/queue/queue.go Outdated Show resolved Hide resolved
modules/distributor/queue/queue.go Outdated Show resolved Hide resolved
@Blinkuu Blinkuu changed the title WIP: Implement generic forwarder Implement generic forwarder Oct 7, 2022
mapno
mapno reviewed Oct 10, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Overall, looks very good to me. I only find the manager logic to be complicated and difficult to follow. Left a comment.

Will wait to see if anyone else wants to chip in.

modules/distributor/forwarder/otlpgrpc/forwarder.go Show resolved Hide resolved
joe-elliott
joe-elliott reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

This is a large PR that will require several hours to give a proper review to. Is there anyway to break the refactor up from the functionality addition?

modules/distributor/distributor.go Show resolved Hide resolved
modules/distributor/distributor.go Show resolved Hide resolved
modules/distributor/forwarder.go Outdated Show resolved Hide resolved
@Blinkuu Blinkuu mentioned this pull request Oct 12, 2022
Merged
3 tasks
@Blinkuu
Copy link
Contributor Author

Blinkuu commented Oct 12, 2022

This PR is now rebased on top of #1796. That PR must be merged before this one.

joe-elliott
joe-elliott reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

I think we can reduce the complexity of this PR significantly. Let me know if some of these comments are unclear.

modules/distributor/distributor.go Outdated Show resolved Hide resolved
modules/distributor/distributor.go Show resolved Hide resolved
modules/distributor/forwarder/config.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/config.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/otlpgrpc/config.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/manager.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/manager.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/manager.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/otlpgrpc/forwarder.go Outdated Show resolved Hide resolved
modules/distributor/forwarder/otlpgrpc/forwarder.go Outdated Show resolved Hide resolved
@Blinkuu Blinkuu requested a review from joe-elliott October 17, 2022 10:00
@joe-elliott joe-elliott self-assigned this Oct 17, 2022
@Blinkuu Blinkuu requested review from joe-elliott and mapno October 18, 2022 11:36
mapno
mapno reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

I think it's almost there. Great work 👏

modules/distributor/forwarder/manager.go Outdated Show resolved Hide resolved
joe-elliott
joe-elliott reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

One final question and then I'm good.

modules/distributor/distributor.go Show resolved Hide resolved
modules/distributor/forwarder/otlpgrpc/forwarder.go Show resolved Hide resolved
mapno
mapno approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM

@mapno mapno merged commit c673a60 into grafana:main Oct 19, 2022
@Blinkuu Blinkuu deleted the implement_generic_forwarder branch October 19, 2022 10:03
This was referenced Nov 8, 2022
Merged
Merged
@mapno mapno mentioned this pull request Dec 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-elliott joe-elliott joe-elliott left review comments

@mapno mapno mapno approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

Assignees

@joe-elliott joe-elliott

@Blinkuu Blinkuu

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Blinkuu @CLAassistant @joe-elliott @mapno @zalegrala