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

add tracking and comparison of latest healthy content before loading a module #5054

Merged
merged 4 commits into from
Sep 1, 2023
Merged

add tracking and comparison of latest healthy content before loading a module #5054

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e196257
add tracking and comparison of latest healthy content before loading …
erikbaranowski Aug 31, 2023
3b93e23
changelog
erikbaranowski Aug 31, 2023
44b523d
don't reset latestContent or set it at all on failure to load module
erikbaranowski Aug 31, 2023
e186f5f
add tracking and check for arguments in module loaders
erikbaranowski Aug 31, 2023
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ Main (unreleased)
- Read contextual attributes from Faro measurements (@codecapitano)
- Rename Grafana Agent service in windows app and features to not include the description

### Bugfixes

- Restart managed components of a module loader only on if module content
changes or the last load failed. This was specifically impacting `module.git`
erikbaranowski marked this conversation as resolved.
Show resolved Hide resolved
each time it pulls. (@erikbaranowski)

v0.36.0 (2023-08-30)
--------------------

Expand Down
27 changes: 24 additions & 3 deletions component/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ type ModuleComponent struct {
opts component.Options
mod component.Module

mut sync.RWMutex
health component.Health
mut sync.RWMutex
health component.Health
latestContent string
}

// Exports holds values which are exported from the run module.
Expand All @@ -38,10 +39,16 @@ func NewModuleComponent(o component.Options) (*ModuleComponent, error) {

// LoadFlowContent loads the flow controller with the current component content. It
// will set the component health in addition to return the error so that the consumer
// can rely on either or both.
// can rely on either or both. If the content is the same as the last time it was
// successfully loaded, it will not be reloaded.
func (c *ModuleComponent) LoadFlowContent(args map[string]any, contentValue string) error {
if contentValue == c.getLatestContent() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check args also?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking of if an argument with a default was not specified and then the argument was added and the content is the same.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds right to me, LoadFlowContent should be a no-op if both the content or the args change. If either of them changed, then it should go through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattdurham, take a peek at what I did in the latest. I'm a little uncomfortable with the arguments being a map[string]any for correct comparison in all cases. do you think we need to do something more significant in how arguments are handled or do you think this covers it for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should work, since DeepEqual errs on the side of return false.

return nil
}

err := c.mod.LoadConfig([]byte(contentValue), args)
if err != nil {
c.setLatestContent("")
erikbaranowski marked this conversation as resolved.
Show resolved Hide resolved
c.setHealth(component.Health{
Health: component.HealthTypeUnhealthy,
Message: fmt.Sprintf("failed to load module content: %s", err),
Expand All @@ -51,11 +58,13 @@ func (c *ModuleComponent) LoadFlowContent(args map[string]any, contentValue stri
return err
}

c.setLatestContent(contentValue)
c.setHealth(component.Health{
Health: component.HealthTypeHealthy,
Message: "module content loaded",
UpdateTime: time.Now(),
})

return nil
}

Expand All @@ -77,3 +86,15 @@ func (c *ModuleComponent) setHealth(h component.Health) {
defer c.mut.Unlock()
c.health = h
}

func (c *ModuleComponent) setLatestContent(content string) {
c.mut.Lock()
defer c.mut.Unlock()
c.latestContent = content
}

func (c *ModuleComponent) getLatestContent() string {
c.mut.RLock()
defer c.mut.RUnlock()
return c.latestContent
}