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

feat: add kernelVersion to telemetry traces #1094

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Anirudh2112
Copy link

Description

This pull request adds the kernelVersion property to telemetry traces in the TrackTrace method. This enhancement allows pinpointing the specific kernel version responsible for producing logs.

To achieve this:

Introduced a kernelVersionFunc variable to enable easier mocking during tests.
Updated the TrackTrace method to include kernelVersion in telemetry traces.
Added a test (TestTrackTraceIncludesKernelVersion) to verify the inclusion of kernelVersion in trace properties.
Related Issue
This pull request addresses the issue:
#1071

Checklist

I have read the contributing documentation
I signed and signed-off the commits (git commit -S -s ...).
I have tested the changes locally.
I have followed the project's style guidelines.
I have added tests.

Screenshots (if applicable) or Testing Completed

Test Output

The following test passes successfully:

  • go test -run TestTrackTraceIncludesKernelVersion

@Anirudh2112 Anirudh2112 requested a review from a team as a code owner December 2, 2024 17:28
@Anirudh2112
Copy link
Author

@microsoft-github-policy-service agree

pkg/telemetry/telemetry.go Outdated Show resolved Hide resolved
pkg/telemetry/telemetry.go Outdated Show resolved Hide resolved
@Anirudh2112 Anirudh2112 force-pushed the add-kernelversion-to-traces branch 2 times, most recently from b9a34f3 to bc6557b Compare December 2, 2024 22:41
pkg/telemetry/telemetry.go Outdated Show resolved Hide resolved
@Anirudh2112
Copy link
Author

Yes, @matmerr that is true, and thank you @rbtr for catching that. I’ve updated the conditional to check for err != nil to avoid infinite recursion. Please let me know if there are any other concerns.

@rbtr
Copy link
Collaborator

rbtr commented Dec 2, 2024

@Anirudh2112 the infinite recursion is still present, trackWarning calls TrackTrace

@Anirudh2112 Anirudh2112 force-pushed the add-kernelversion-to-traces branch from 70fd88e to ad29b29 Compare December 2, 2024 23:33
@Anirudh2112
Copy link
Author

Apologies for the oversight regarding the infinite recursion. In the recent changes, I’ve addressed this by:

  1. Replacing the call to t.trackWarning with direct error logging to avoid recursion.
  2. Ensuring that errors are logged as separate telemetry traces using appinsights.NewTraceTelemetry.
  3. Preserving the original functionality by adding kernelVersion to the properties map when available and maintaining the flow for the main trace.

@Anirudh2112 Anirudh2112 force-pushed the add-kernelversion-to-traces branch from 503c6d6 to f514bbb Compare December 5, 2024 23:42
@Anirudh2112 Anirudh2112 requested a review from matmerr December 14, 2024 19:46
@Anirudh2112
Copy link
Author

Hi @matmerr @rbtr, I've addressed all the review comments on my PRs. Just wanted to check if there are any other issues I should address or if there's anything else needed from my side to help move these forward. Thanks!

There's no particular reason why this KernelVersion platform detection
has to be done at compile time. It complicates testing, as the function
is redefined by build tags, so the only way to avoid invoking commands
on the host is to have a throwaway func var to assign KernelVersion to.

It's completely reasonable to think that there's other information that
we might want to grab from the host eventually, so this really should
reside in a client of some kind. This patch adds such a client and makes
KernelVersion a method of it. This method uses runtime.GOOS to decide at
runtime which command will be invoked based on the platform found.

This opens the door for using an interface to mock this method more
cleanly in tests that desire to do so. Those tests have been updated.

Finally, the global `KernelVersion` func has been removed in favor of
instantiating one of these HostInfoClients instead (the surface area of
`KernelVersion` was very small, so the refactor was trivial).
@timraymond
Copy link
Member

@Anirudh2112 I had a few suggestions, mostly around the use of the global var to mock the KernelVersion func. Two issues with it: 1) it makes the tests racy, which limits the amount of concurrency we can use when running the test suite. Fast tests are nice, and you can only really have fast tests by staying vigilant for things that slow them down :) 2) Depending on the KernelVersion func is really the wrong dependency to take--what we want to depend on the behavior of "give me the kernel version from this host." The way to depend on behavior in Go is to depend on an interface.

Looking at the patch closer though, I can see what you had to work with, and a lot of it was pre-existing. I went ahead and made the changes I suggested to save some back-and-forth. The commit is on traymond/kernelversion-traces. Assuming the typical "triangular workflow," you should be able to fetch it from your upstream remote, then git merge --ff-only onto your branch to pull the change in.

@Anirudh2112
Copy link
Author

@timraymond I've merged your changes from the traymond/kernelversion-traces branch. I appreciate the improvements made to handle platform detection at runtime and the cleaner approach using the HostInfoClient. I've reviewed the changes and understand this removes the need for compile-time platform detection while making the code more testable. Please let me know if there's anything else needed.

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.

4 participants