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

Add queue name to state reporting #7537

Closed
wants to merge 1 commit into from
Closed

Add queue name to state reporting #7537

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libbeat/publisher/pipeline/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ func createQueueBuilder(config common.ConfigNamespace) (func(queue.Eventer) (que
queueConfig = common.NewConfig()
}

stateRegistry := monitoring.GetNamespace("state").GetRegistry()
queueRegistry := stateRegistry.NewRegistry("queue")
monitoring.NewString(queueRegistry, "name").Set(queueType)
Copy link

Choose a reason for hiding this comment

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

The pipeline should not use any globals at all, so one can safely instantiate multiple pipelines via Load. The Load function even gets the metrics registry passed as parameter (optional, can be nil).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The registry passed is not the same into which this data has to go.

If there can be multiple queues in the future we would need a variable inside this package to keep track of it like we do for modules. Would that be ok for you?

Copy link

Choose a reason for hiding this comment

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

I understand it's a different regsitry. In this case we should pass a 'telemetry' registry or some kind of callback to the loader. But no globals. It's one of the few packages with no globals in mind from the very beginning.

Me might have other places where we need to pass a telemetry and a metrics registry to a feature/module/plugin. Maybe we want to pass a struct or interface of type:

type monitoring interface {
  Telemetry() *monitoring.Registry
  Metrics() *monitoring.Registry
  Logs() logp.Logger
}

Copy link

Choose a reason for hiding this comment

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

@ruflin I see some globals have been sneaked in... see #8091 for cleanups + introduction of queue type telemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for opening #8091 It seems this make this PR obsolete, right?

Copy link

Choose a reason for hiding this comment

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

Yes.


return func(eventer queue.Eventer) (queue.Queue, error) {
return queueFactory(eventer, queueConfig)
}, nil
Expand Down