[AppSignals] Implement translation for appsignals on native k8s #1012
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
+ Coverage 57.58% 63.30% +5.72%
==========================================
Files 370 361 -9
Lines 17548 18199 +651
==========================================
+ Hits 10105 11521 +1416
+ Misses 6848 6083 -765
Partials 595 595 ☔ View full report in Codecov by Sentry. |
JayPolanco
force-pushed
the
appsignalk8s
branch
from
11a2ba0 to
c1c40b8
Compare
lisguo
reviewed
Jan 19, 2024
plugins/processors/awsappsignals/internal/resolver/attributesresolver.go
Outdated
Show resolved
Hide resolved
Comment on lines
+70
to
+81
| isEks, err := common.IsEKS() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if isEks { | ||
| cfg.Resolvers = []appsignalsconfig.Resolver{ | ||
| appsignalsconfig.NewEKSResolver(hostedIn), | ||
| } | ||
| } else { | ||
| cfg.Resolvers = []appsignalsconfig.Resolver{ | ||
| appsignalsconfig.NewK8sResolver(hostedIn), |
There was a problem hiding this comment.
I see this code snippet used in a bunch of places ? can it be moved to a common util package ?
There was a problem hiding this comment.
The logic within the if else blocks are all different. The common code would be lines 70 to 73, but that can't be exported because we would still need to return if an error occurs.
lisguo
approved these changes
Jan 19, 2024
mitali-salvi
approved these changes
Jan 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the issue
Native kubernetes(k8s) requires a different configuration for appsignals. This PR handles the translation for when the agent is on a native k8s cluster
Description of changes
HostedIn.K8s.Clusterwhen on native k8s instead ofHostedIn.EKS.ClusterLicense
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Unit tested. Tested on native kubernetes cluster using minikube. Translated yaml for both native k8s and EKS attached below:
Note: ContainerInsights was disabled to remove clutter when viewing translated yaml
Native k8s:
EKS (No changes made, this snippet is just for confirmation):
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint