Update gosigar package by narph · Pull Request #28909 · elastic/beats

narph · 2021-11-10T14:20:45Z

What does this PR do?

Updates gosigar lib which has fix for filesystem windows error
Update docs with limitation on external disks
Stats are also unavailable in these situations.

Why is it important?

Updates gosigar lib which has fix for filesystem windows error

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Relates Handle ERROR_INVALID_PARAMETER error for GetVolumeInformationW win32 api gosigar#164

Ex:

{
  "@timestamp": "2021-11-10T15:21:32.613Z",
  "@metadata": {
    "beat": "metricbeat",
    "type": "_doc",
    "version": "8.1.0"
  },
  "ecs": {
    "version": "8.0.0"
  },
  "host": {
    ...
  },
  "agent": {
   ...
  },
  "event": {
..
  },
  "metricset": {
    "name": "filesystem",
    "period": 10000
  },
  "service": {
    "type": "system"
  },
  "system": {
    "filesystem": {
      "used": {
        "bytes": 513627078656,
        "pct": 0.5115
      },
      "type": "ntfs",
      "device_name": "C:\\",
      "mount_point": "C:\\",
      "total": 1004205502464,
      "available": 490578423808,
      "free": 490578423808
    }
  }
}


{
  "@timestamp": "2021-11-10T15:21:32.613Z",
  "@metadata": {
    "beat": "metricbeat",
    "type": "_doc",
    "version": "8.1.0"
  },
  "host": {
   ....
  },
  "agent": {
  ...
  },
  "system": {
    "filesystem": {
      "available": 0,
      "free": 0,
      "used": {
        "pct": 0,
        "bytes": 0
      },
      "type": "unavailable",
      "device_name": "D:\\",
      "mount_point": "D:\\",
      "total": 0
    }
  },
  "event": {
    "module": "system",
    "duration": 7789600,
    "dataset": "system.filesystem"
  },
  "metricset": {
    "name": "filesystem",
    "period": 10000
  },
  "service": {
    "type": "system"
  },
  "ecs": {
    "version": "8.0.0"
  }
}

mergify · 2021-11-10T14:21:23Z

This pull request does not have a backport label. Could you fix it @narph? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

mergify · 2021-11-10T14:39:33Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-incorrect upstream/add-incorrect
git merge upstream/master
git push upstream add-incorrect

elasticmachine · 2021-11-10T15:11:58Z

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-11-15T15:39:56.227+0000
Duration: 128 min 11 sec
Commit: 398b1c6

Test stats 🧪

Test	Results
Failed	0
Passed	53696
Skipped	5233
Total	58929

Steps errors

Expand to view the steps failures

`x-pack/metricbeat-goIntegTest - mage goIntegTest`

Took 16 min 6 sec . View more details here
Description: mage goIntegTest

`Recursively delete the current directory from the workspace`

Took 0 min 2 sec . View more details here
Description: Unable to delete '/var/lib/jenkins/workspace/PR-28909-5-3e778349-9aa5-417c-b912-7fb73b993a1b'. Tried

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2021-11-10T15:29:48Z

Pinging @elastic/integrations (Team:Integrations)

fearful-symmetry · 2021-11-10T20:04:18Z

@@ -112,8 +115,10 @@ func filterFileSystemList(fsList []sigar.FileSystem) []sigar.FileSystem {
 // GetFileSystemStat retreves stats for a single filesystem
 func GetFileSystemStat(fs sigar.FileSystem) (*FSStat, error) {
 	stat := sigar.FileSystemUsage{}


I still wonder if we should log this somewhere. Maybe in the main filesystem.go?

my worry is flooding the logs with this warning/error message, knowing folks will collect these metrics in short intervals.
I would avoid it for now and keep it in the documentation. If we get any user feedback about any confusion there, then we might need to reconsider.

jsoriano · 2021-11-11T14:53:06Z

 	stat := sigar.FileSystemUsage{}
-	if err := stat.Get(fs.DirName); err != nil {
-		return nil, err
+	if fs.SysTypeName != UnavailableDisk {


Do you have a sample event when this happens? Are stats populated with zero values?

no stats json object, the example is right in this ticket description

Oh ok, I see. There are all populated with zeroes:

"filesystem": { "available": 0, "free": 0, "used": { "pct": 0, "bytes": 0 }, "type": "unavailable", "device_name": "D:\\", "mount_point": "D:\\", "total": 0 }

This may be confusing, for example when preparing alerts, you may need to use other fields to check if the available space is 0 because the filesystem is actually full or because the metric was not available. I think that in these cases these metrics shouldn't be reported:

"filesystem": { "type": "unavailable", "device_name": "D:\\", "mount_point": "D:\\", }

The event may still be useful for inventory reasons.

somehow I thought the stats would not be there.
I had a look at the function retrieving the stats and it is reused by the fsstat metricset. The fsstat metricset logs the exception as a debug message and continues to the next disk. I tried to do the same for the filesystem for consistency purposes.

So the debug logs will show:

Line 46: {"log.level":"debug","@timestamp":"2021-11-15T16:26:13.465+0100","log.logger":"system.fsstat","log.origin":{"file.name":"fsstat/fsstat.go","file.line":85},"message":"error fetching filesystem stats for 'D:\\': GetDiskFreeSpaceEx failed: The device is not ready.","service.name":"metricbeat","ecs.version":"1.6.0"} Line 47: {"log.level":"debug","@timestamp":"2021-11-15T16:26:13.465+0100","log.logger":"system.filesystem","log.origin":{"file.name":"filesystem/filesystem.go","file.line":85},"message":"error fetching filesystem stats for 'D:\\': GetDiskFreeSpaceEx failed: The device is not ready.","service.name":"metricbeat","ecs.version":"1.6.0"}

nothing changed for fsstat
but for filesystem events will look like this:

{ "_index" : "metricbeat-8.1.0-2021.11.15-000001", "_type" : "_doc", "_id" : "mis1JH0BLuBcqFZ9zBxN", "_score" : null, "_source" : { "@timestamp" : "2021-11-15T15:27:24.244Z", "system" : { "filesystem" : { "type" : "ntfs", "device_name" : """C:\""", "mount_point" : """C:\""", "total" : 1004205502464, "available" : 477790535680, "free" : 477790535680, "used" : { "bytes" : 526414966784, "pct" : 0.5242 } } }, "ecs" : { "version" : "8.0.0" }, "host" : { "name" : "DESKTOP-K76UDQL" }, "agent" : { ... }, "metricset" : { "name" : "filesystem", "period" : 10000 }, "event" : { "dataset" : "system.filesystem", "module" : "system", "duration" : 12886200 }, "service" : { "type" : "system" } }, "sort" : [ 1636990044244 ] }, { "_index" : "metricbeat-8.1.0-2021.11.15-000001", "_type" : "_doc", "_id" : "nCs1JH0BLuBcqFZ9zBxN", "_score" : null, "_source" : { "@timestamp" : "2021-11-15T15:27:24.244Z", "agent" : { ... }, "event" : { "dataset" : "system.filesystem", "module" : "system", "duration" : 21079100 }, "metricset" : { "name" : "filesystem", "period" : 10000 }, "service" : { "type" : "system" }, "system" : { "filesystem" : { "type" : "unavailable", "device_name" : """D:\""", "mount_point" : """D:\""" } }, "ecs" : { "version" : "8.0.0" }, "host" : { "name" : "DESKTOP-K76UDQL" } }, "sort" : [ 1636990044244 ] }

@fearful-symmetry , @jsoriano wdyt?

Agreeing with @jsoriano , we shouldn't report "null zero" metrics. The fields should be omitted where possible, and I think having "bare" events with no/few metrics in this case isn't terrible.

jsoriano · 2021-11-11T14:54:28Z

+	if fs.SysTypeName != UnavailableDisk {
+		if err := stat.Get(fs.DirName); err != nil {
+			return nil, err
+		}
 	}


I wonder if we should just ignore this error if we want to always report some event even if these stats are not available.

Suggested change

if fs.SysTypeName != UnavailableDisk {

if err := stat.Get(fs.DirName); err != nil {

return nil, err

}

}

if fs.SysTypeName != UnavailableDisk {

if err := stat.Get(fs.DirName); err != nil {

log.Debugf("....: %v", err)

}

}

same here, behavior would more consistent

narph · 2021-11-15T10:09:49Z

/package

jsoriano

LGTM, please wait for @fearful-symmetry opinion.

jsoriano · 2021-11-15T15:53:22Z

-		"free":        fsStat.Free,
-		"used": common.MapStr{
+	}
+	if addStats == true {


Nit. Redundant boolean comparison.

Suggested change

if addStats == true {

if addStats {

narph · 2021-11-15T18:14:57Z

/package

fearful-symmetry

LGTM, I think that bool check so we don't report a bunch of null metrics is a reasonable solution.

narph · 2021-11-16T08:23:08Z

/package

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0) # Conflicts: # metricbeat/module/system/fields.go

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0)

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0) Co-authored-by: Mariana Dima <mariana@elastic.co>

* update gosigar * text changes * check for unavailable stats * new changes

* Update gosigar package (#28909) * update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0) # Conflicts: # metricbeat/module/system/fields.go * Update gosigar package (#28909) * update gosigar * text changes * check for unavailable stats * new changes Co-authored-by: Mariana Dima <mariana@elastic.co>

update gosigar

41a9627

botelastic Bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 10, 2021

mergify Bot added the backport-skip Skip notification from the automated backport with mergify label Nov 10, 2021

narph self-assigned this Nov 10, 2021

text changes

58f8523

Merge branch 'master' into add-incorrect

9698fe6

check for unavailable stats

d1dc64d

narph requested a review from jsoriano November 10, 2021 15:29

narph added the Team:Integrations Label for the Integrations team label Nov 10, 2021

botelastic Bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 10, 2021

narph requested a review from fearful-symmetry November 10, 2021 15:29

narph added the backport-v7.15.0 Automated backport with mergify label Nov 10, 2021

mergify Bot removed the backport-skip Skip notification from the automated backport with mergify label Nov 10, 2021

narph added backport-v7.16.0 Automated backport with mergify and removed backport-v7.15.0 Automated backport with mergify labels Nov 10, 2021

fearful-symmetry reviewed Nov 10, 2021

View reviewed changes

narph added the backport-v8.0.0 Automated backport with mergify label Nov 11, 2021

jsoriano reviewed Nov 11, 2021

View reviewed changes

narph requested review from fearful-symmetry and jsoriano November 15, 2021 10:42

new changes

398b1c6

jsoriano approved these changes Nov 15, 2021

View reviewed changes

fearful-symmetry approved these changes Nov 15, 2021

View reviewed changes

narph merged commit 08642d0 into elastic:master Nov 16, 2021

narph deleted the add-incorrect branch November 16, 2021 15:57

mergify Bot pushed a commit that referenced this pull request Nov 16, 2021

Update gosigar package (#28909)

3595644

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0) # Conflicts: # metricbeat/module/system/fields.go

mergify Bot mentioned this pull request Nov 16, 2021

[7.16](backport #28909) Update gosigar package #28999

Merged

mergify Bot pushed a commit that referenced this pull request Nov 16, 2021

Update gosigar package (#28909)

82af725

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0)

mergify Bot mentioned this pull request Nov 16, 2021

[8.0](backport #28909) Update gosigar package #29000

Merged

narph added a commit that referenced this pull request Nov 17, 2021

Update gosigar package (#28909) (#29000)

41b6e23

* update gosigar * text changes * check for unavailable stats * new changes (cherry picked from commit 08642d0) Co-authored-by: Mariana Dima <mariana@elastic.co>

narph added a commit that referenced this pull request Nov 17, 2021

Update gosigar package (#28909)

ce640fa

* update gosigar * text changes * check for unavailable stats * new changes

Conversation

narph commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Related issues

Uh oh!

mergify Bot commented Nov 10, 2021

Uh oh!

mergify Bot commented Nov 10, 2021

Uh oh!

elasticmachine commented Nov 10, 2021 • edited by jenkins-beats-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Build stats

Test stats 🧪

Steps errors

x-pack/metricbeat-goIntegTest - mage goIntegTest

Recursively delete the current directory from the workspace

💚 Flaky test report

🤖 GitHub comments

Uh oh!

elasticmachine commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narph commented Nov 15, 2021

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narph commented Nov 15, 2021

Uh oh!

fearful-symmetry left a comment

Choose a reason for hiding this comment

Uh oh!

narph commented Nov 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

narph commented Nov 10, 2021 •

edited

Loading

elasticmachine commented Nov 10, 2021 •

edited by jenkins-beats-ci Bot

Loading

`x-pack/metricbeat-goIntegTest - mage goIntegTest`

`Recursively delete the current directory from the workspace`