-
Notifications
You must be signed in to change notification settings - Fork 97
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
Use informer code from beyla-k8s-cache #1256
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1256 +/- ##
==========================================
+ Coverage 78.12% 81.71% +3.58%
==========================================
Files 135 131 -4
Lines 11384 11041 -343
==========================================
+ Hits 8894 9022 +128
+ Misses 1951 1505 -446
+ Partials 539 514 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my comments are minor and pedantic - feel free to ignore. This looks really good. Way to go, thanks for this!
go.mod
Outdated
@@ -1,6 +1,6 @@ | |||
module github.com/grafana/beyla | |||
|
|||
go 1.23 | |||
go 1.23.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if relevant here, but perhaps we should double check if this has any implications with Alloy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was automatically added by go mod tidy
. Let me just revert this line.
pkg/internal/appolly/appolly.go
Outdated
// caching the database initialization. Discarding it as subsequent | ||
// invocations to Get will return the same initialized instance | ||
_, err := ctxInfo.K8sInformer.Get(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely subjective and arguable suggestion: perhaps we can do:
func refreshK8sInformerCache(ctx ...) error {
_, err: ctxInfo.K8sInformer.Get(ctx) // force the cache to be populated/refreshed
return err
}
err := refreshK8sInformerCache(ctx)
just so it is a tad more idiomatic - but like I said, feel free to ignore as this is a very subjective approach
|
||
return nil | ||
// On is invoked every time an object metadata instance is stored or deleted in the | ||
// kube.Store. It will just forward the event via channel for proper asynchronous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// kube.Store. It will just forward the event via channel for proper asynchronous | |
// kube.Store. It will just forward the event via the channel for proper asynchronous |
As a previous step to allow caching the informers' metadata into an intermediate service, this PR replaces the informers' code by the one used in the https://github.com/grafana/beyla-k8s-cache project.
This also allowed to simplify the informers code, as the previous code was a bit cumbersome and difficult to maintain, being a mixture of two informers (network and app), with some patch-over-patch modifications.