-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Removing obsolete package from metricbeat #35827
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
@@ -43,7 +43,7 @@ func eventMapping(r mb.ReporterV2, cont *types.Container, m *MetricSet) { | |||
|
|||
container, err := m.dockerClient.ContainerInspect(context.TODO(), cont.ID) | |||
if err != nil { | |||
errors.Wrapf(err, "Error inspecting container %v", cont.ID) | |||
fmt.Errorf("Error inspecting container %v: %w", cont.ID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Special attention here
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM, the one place the error was unused feels like it is a bug that could be improved but within the scope of what this PR is trying to do it could also be left as is.
@@ -43,7 +42,7 @@ func eventMapping(r mb.ReporterV2, cont *types.Container, m *MetricSet) { | |||
|
|||
container, err := m.dockerClient.ContainerInspect(context.TODO(), cont.ID) | |||
if err != nil { | |||
errors.Wrapf(err, "Error inspecting container %v", cont.ID) | |||
// errors.Wrapf(err, "Error inspecting container %v", cont.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it might be a bug that this is just getting dropped.
This is called from the following function which does allow returning an error:
beats/metricbeat/module/docker/healthcheck/healthcheck.go
Lines 67 to 76 in 3325ba1
func (m *MetricSet) Fetch(r mb.ReporterV2) error { | |
// Fetch a list of all containers. | |
containers, err := m.dockerClient.ContainerList(context.TODO(), types.ContainerListOptions{}) | |
if err != nil { | |
return errors.Wrap(err, "failed to get docker containers list") | |
} | |
eventsMapping(r, containers, m) | |
return nil | |
} |
The only trick would be that you don't want to just return an error and immediately stop if one of many containers has an error.
beats/metricbeat/module/docker/healthcheck/data.go
Lines 33 to 37 in 3325ba1
func eventsMapping(r mb.ReporterV2, containers []types.Container, m *MetricSet) { | |
for _, container := range containers { | |
eventMapping(r, &container, m) | |
} | |
} |
For this you could use https://github.com/hashicorp/go-multierror there's an example here
err = multierror.Append(err, fmt.Errorf("Config for '%s' is blacklisted", configs.Type)) |
This multierr package itself can probably be replaced with standard library functionality once we upgrade to Go 1.20.
If you don't want to deal with all that, you can just delete this. It isn't making the code any worse and it was unused before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, deleting. will open a follow up issue to fix this.
Team, |
github.com/pkg/errors is deprecated since 2021.
Furthermore, it's at least 100% worse in terms of performance compared to the native errors and fmt packages, (probably due to the fact every error log comes with a stack trace)
Another interesting benchmark: https://g4s8.wtf/posts/go-errors-performance/