Add livedebugging support for discovery components #2309
Conversation
ravishankar15
requested review from
clayton-cornell and
a team
as code owners
| @@ -112,6 +112,36 @@ Supported components: | |||
| * `prometheus.relabel` | |||
| {{< /admonition >}} | |||
| * `discovery.relabel` | |||
There was a problem hiding this comment.
We can probably abbreviate this to discovery.* unless there's discovery components not added by this PR.
There was a problem hiding this comment.
Abbreviating would follow the pattern for other component groups.
| // New creates a discovery component given arguments and a concrete Discovery implementation function. | ||
| func New(o component.Options, args component.Arguments, creator Creator) (*Component, error) { | ||
| debugDataPublisher, err := o.GetServiceData(livedebugging.ServiceName) | ||
| if err != nil { |
There was a problem hiding this comment.
If there is an error in connecting with livedebugging I would not expect that to cause the component to fail to instantiate.
Just looked, and this is clearly the established pattern so I'll bring that up in a separate channel, this is fine for now.
There was a problem hiding this comment.
yeah I agree that it is a bit weird that a missing debugging functionality would prevent the components from starting. But because this is not something that can happen (all services are always created at start), we decided to always expect the service to be there. We could revisit it by adding nil pointers check instead of falling + some way to log once that the live debugging functionality is not available
| @@ -257,3 +273,5 @@ func (c *Component) runDiscovery(ctx context.Context, d DiscovererWithMetrics) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func (c *Component) LiveDebugging(_ int) {} | |||
There was a problem hiding this comment.
My understanding is that this function is triggered on a change of livedebug consumers. Should the function Publish the cached targets data if the number of consumers > 0 as discovery may not update rapidly otherwise? CC @wildum
There was a problem hiding this comment.
I don't think so because the concept of live debugging is to show what's happening now. If I have live debugging open and I see targets I expect that the targets were just sent. The user can see the last sent targets in the exports area of the component page in the UI.
We can revisit this concept if that's something users really want to see on the live graph/live debugging feed but we could simply just use the export values
clayton-cornell
added
the
type/docs
| * `discovery.nomad` | ||
| * `discovery.openstack` | ||
| * `discovery.ovhcloud` | ||
| * `discovery.process` |
There was a problem hiding this comment.
discovery process is not using the common discovery component. Can you add livedebugging to it too please?
PR Description
Which issue(s) this PR fixes
Fixes #1034
Notes to the Reviewer
PR Checklist