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

Add ECS fields to process & FS resources #254

Merged
merged 23 commits into from
Jul 10, 2022

Conversation

uri-weisman
Copy link
Contributor

@uri-weisman uri-weisman commented Jul 6, 2022

@mergify
Copy link

mergify bot commented Jul 6, 2022

This pull request does not have a backport label. Could you fix it @uri-weisman? 🙏
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 8./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip label Jul 6, 2022
@uri-weisman uri-weisman changed the title Add ECS fields to process resource Add ECS fields to process & FS resources Jul 6, 2022
@uri-weisman uri-weisman marked this pull request as ready for review July 6, 2022 11:44
@uri-weisman uri-weisman requested review from a team as code owners July 6, 2022 11:44
@@ -40,15 +43,65 @@ const (
CMDArgumentMatcher = "\\b%s[\\s=]\\/?(\\S+)"
ProcessResourceType = "process"
ProcessSubType = "process"
userHz = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

The value of HZ varies across kernel versions and hardware platforms.
There is an assumption here for certain conditions that might not be respected between different deployments and versions.
I think that we can use https://man7.org/linux/man-pages/man3/sysconf.3.html and pull the value of _SC_CLK_TCK in order to reduce this assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for raising this up @jeniawhite .
It is a borrowed implementation from go-sysinfo, we decided to deliver it with this known limitation while opening 2 new tickets to the go-sysinfo repo.

Once those tasks will be delivered, possibly by us, we'll change our implementation to use the go-sysinfo pkg.

@jeniawhite jeniawhite self-requested a review July 7, 2022 11:49
Copy link
Contributor

@DaveSys911 DaveSys911 left a comment

Choose a reason for hiding this comment

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

LGTM

@uri-weisman uri-weisman enabled auto-merge (squash) July 10, 2022 12:00
@DaveSys911 DaveSys911 self-requested a review July 10, 2022 12:01
@@ -78,6 +78,9 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/setup-go@v3
with:
go-version: 1.18
Copy link
Contributor

@DaveSys911 DaveSys911 Jul 10, 2022

Choose a reason for hiding this comment

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

Could you please set go version using the env var like the unit tests job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -80,6 +80,8 @@ func (r K8sResource) GetMetadata() fetching.ResourceMetadata {
}
}

func (r K8sResource) GetElasticCommonData() any { return nil }
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this sit in the kube_fetcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems fine to me to reside next to the rest of the Resource interface

type Resource interface {
	GetMetadata() ResourceMetadata
	GetData() any
	GetElasticCommonData() any
}

If you feel this is not the place we can move them all.

@uri-weisman uri-weisman merged commit 15cd261 into elastic:main Jul 10, 2022
@uri-weisman uri-weisman deleted the ecs_process branch July 11, 2022 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants