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

Make loki.LogsReceiver an interface #4303

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented Jun 29, 2023

PR Description

In order to implement config converter we need to override how a components' export is tokenized.

  • There is an example of how this is done here for prometheus.remote_write - when we create a block for remote_write, we return an remotewrite.Exports.
  • That export is then set on river components args that use it and since we override the way its tokenized, we control how it is displayed in the final River configuration that is generated. In the example above, the exported receiver will be displayed as "prometheus.remote_write.default.receiver" which is what we want.
  • However, the trick to override how an export value is tokenized requires us to extend the exported type, which can only be done when it's an interface.
  • loki.LogsReceiver is not an interface, so I ran into an issue when trying to implement promtail config conversion support. The problem can be seen in code in this draft PR.

Proposed solution: make loki.LogsReceiver an interface.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@thampiotr thampiotr marked this pull request as ready for review June 29, 2023 12:45
@thampiotr thampiotr requested review from a team as code owners June 29, 2023 12:45
@thampiotr thampiotr changed the title WIP: Make loki.LogsReceiver an interface Make loki.LogsReceiver an interface Jun 29, 2023
Comment on lines +26 to +48
// LogsReceiver is an interface providing `chan Entry` which is used for component
// communication.
type LogsReceiver chan Entry
type LogsReceiver interface {
Chan() chan Entry
}

type logsReceiver struct {
entries chan Entry
}

func (l *logsReceiver) Chan() chan Entry {
return l.entries
}

func NewLogsReceiver() LogsReceiver {
return NewLogsReceiverWithChannel(make(chan Entry))
}

func NewLogsReceiverWithChannel(c chan Entry) LogsReceiver {
return &logsReceiver{
entries: c,
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change - the rest is just making it work using it.

@tpaschalis tpaschalis self-requested a review June 29, 2023 12:50
@mattdurham
Copy link
Collaborator

Might be best for a separate PR, but I am wondering if its possible to hide the underlying implementation and the interface be more generic while we are looking at it.

type LogsReceiver interface {
	Send(Entry)
        Receive() Entry
}

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

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

@thampiotr
Copy link
Contributor Author

Might be best for a separate PR, but I am wondering if its possible to hide the underlying implementation and the interface be more generic while we are looking at it.

type LogsReceiver interface {
	Send(Entry)
        Receive() Entry
}

There's also some places where close(channel) is called, so not 100% sure we can hide that it's a channel and hence it may not be useful to do that.

@tpaschalis tpaschalis merged commit 29131a6 into main Jun 29, 2023
@tpaschalis tpaschalis deleted the thampiotr/make-logs-receiver-interface branch June 29, 2023 15:15
@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 23, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

4 participants