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

Write out error message when plugin seems have a bug #1024

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Conversation

rmatsuoka
Copy link
Member

@rmatsuoka rmatsuoka commented Nov 18, 2024

Mackerel-agent does not write out any error log when fails to parse a broken JSON as a plugin's graph definition.
It makes hard to find out a bug of the plugin.

This PR makes mackerel-agent write out error log when plugin seems have a bug.

Implements

I defined PluginFaultError.

// PluginFaultError may be returned by [PluginGenerator.PrepareGraphDefs].
// This error indicates a bug in a plugin and should be logged for a user.
// Note that [PluginGenerator.PrepareGraphDefs] can also return other error types.
type PluginFaultError struct {
	Err error
}

A method PrepareGraphDefs fetches a graph definition from a plugin. By this PR, PrepareGraphDefs will return PluginFaultError, when it faces following two situation,

  • Fails to exec plugin
  • Fails to parse JSON as a graph definition.

Mackerel-agent will write out an error log when it catches PluginFaultError.

Behaviour

When plugin provides broken JSON as a graph definition, mackerel-agent write out ERROR log.

2024/11/18 20:58:40 agent.go:105: ERROR <agent> Failed to fetch meta information from plugin &{0x14000140b60 <nil>}; seems that the plugin may have a bug: while reading plugin configuration: invalid character '}' looking for beginning of object key string

When plugin has no graph definition (It should not be a bug), no ERROR log.

DEBUG log is written out if verbose option is on. This behavior is the same as before.

2024/11/18 20:56:38 agent.go:102: DEBUG <agent> Failed to fetch meta information from plugin &{0x140001c9b90 <nil>} (non critical); seems that this plugin does not have meta information: bad format of first line: "x-custom-sh.testmetric2\t16452\t1731930998\n"

@rmatsuoka rmatsuoka marked this pull request as ready for review November 19, 2024 02:21
Copy link
Contributor

@yohfee yohfee left a comment

Choose a reason for hiding this comment

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

LGTM!

Just out of curiosity, why aren't following situations considered PluginFaultError?
I thought they are also caused when plugin does not satisfy specifications.

  • plugin outputs bad header
  • plugin outputs unsupported version

if err != nil {

var faultError *metrics.PluginFaultError
if errors.As(err, &faultError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] According to https://pkg.go.dev/errors#As example, maybe it is more idiomatic to check nil first then detect by errors.As.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, I feel like it is not idiomatic. I think the way in the example is just for illustration. That requires more nesting.

@rmatsuoka
Copy link
Member Author

rmatsuoka commented Nov 20, 2024

@yohfee

  • plugin outputs bad header

For mackerel-agent, "Bad header" means "the first line from a plugin is not # mackerel-agent-plugin".
This situation is probably that the plugin has no graph definitions and just yields a metric, which is formatted into {metric_name} {metric_value} {timestamp}. It is permissible for a plugin to have no graph definitions, and it should be not a bug.

  • plugin outputs unsupported version

PluginFaultError is yielded when a plugin seems have a bug. Outputting unsupported version is not a bug.

Though, I think it is kind for user to log a message about plugin is unsupported.

@rmatsuoka
Copy link
Member Author

Thanks for your review!

@rmatsuoka rmatsuoka merged commit f26b61c into master Nov 20, 2024
18 checks passed
@rmatsuoka rmatsuoka deleted the log-plugin-bug branch November 20, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants