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

Added a container id provider that supports getting info from mountinfo #273

Merged

Conversation

xiaomi7732
Copy link
Member

Addresses #246

Looks like /proc/self/mountinfo is a relative reliable spot to getting container id that supports cgroupv2.

  • Piggy back on it, I also bumped up the language used in the project to C# 10.
  • A small improvement on logging.

@xiaomi7732
Copy link
Member Author

@pkoelbl This is the PR to implement getting container id from mountinfo. Let me know if you are willing to share any early feedback.

@pkoelbl
Copy link

pkoelbl commented May 27, 2022

Looks good. Let me know when you have a beta nuget

Copy link
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

I have some questions...

@xiaomi7732
Copy link
Member Author

Hey @karolz-ms Thanks so much for the review. I really appreciated that you bring containerd into our attention. I've given it another iteration, mind take another review? Alone with the implementation, I also put up a draft for a wiki of the design we discussed, mind also give it a quick peak: https://github.com/microsoft/ApplicationInsights-Kubernetes/wiki/Design---ContainerIdProviders-%5BDRAFT%5D when you have time? Thanks.

Copy link
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

Looks great1 Just one suggestion, but I am fine with leaving things as they are for now.

I also read through the design doc and it makes a lot of sense to me.

@xiaomi7732 xiaomi7732 merged commit 6b19fd5 into microsoft:develop Jun 6, 2022
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.

None yet

3 participants