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

Report kubernetes system metadata #741

Merged
merged 4 commits into from
Apr 28, 2020
Merged

Report kubernetes system metadata #741

merged 4 commits into from
Apr 28, 2020

Conversation

vhatsura
Copy link
Contributor

@vhatsura vhatsura commented Feb 26, 2020

closes #181

TBD:

  • tests
  • documentation

@vhatsura
Copy link
Contributor Author

vhatsura commented Mar 1, 2020

@bmorelli25, I don't sure do we need to add it into documentation? I found nothing in other agents related to it.

@bmorelli25
Copy link
Member

We document Kubernetes metadata centrally, in APM Server: https://www.elastic.co/guide/en/apm/server/master/metadata-api.html. I don't think anything is needed in this PR 👍

@vhatsura
Copy link
Contributor Author

@gregkalapos, I'll very much appreciate it if you can review the PR.

@gregkalapos
Copy link
Contributor

Hey @vhatsura - sorry for the delay, currently we are fighting with some CI issues; hopefully we can get back to normal soon. Once master is green again I'll make sure PRs gets reviewed and merged.

@vhatsura
Copy link
Contributor Author

@gregkalapos, just a kindly reminder. Would be nice to have such feature in the next release

@ejsmith
Copy link

ejsmith commented Apr 23, 2020

Bump

@apmmachine
Copy link
Contributor

apmmachine commented Apr 28, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 13495
Skipped 0
Total 13495

Steps errors

Expand to view the steps failures

  • Name: Docker Build
    • Description:

    • Result: FAILURE

    • Duration: 0 min 4 sec<

    • Start Time: 2020-04-28T19:42:40.152+0000

Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Thanks @vhatsura - looks good to me.

Some of these can be also collected by parsing /proc/self/cgroup - this is a super useful comment on that. Here is some more info on that.

The Go PR has some sample /proc/self/cgroup that we could also steal for testing.

This is already good progress and I think it's ok to merge - if you feel you can follow up with parsing /proc/self/cgroup later.


return new Api.System
{
Container = containerInfo, DetectedHostName = hostName, Kubernetes = ParseKubernetesInfo(containerInfo, hostName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have either called all 3 methods here directly or also stored the result of ParseKubernetesInfo(containerInfo, hostName) in a variable and assigned that to Kubernetes similar to the two other properties.

But that's just a cosmetic thing - this is totally fine this way too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, it makes sense. Fixed

@gregkalapos gregkalapos merged commit 1ce7c80 into elastic:master Apr 28, 2020
@vhatsura vhatsura deleted the system/kubernetes_info branch April 29, 2020 10:29
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.

Report kubernetes system metadata
5 participants