Create telemetry resource by default for applications #529
Conversation
WalkthroughThe changes introduce new configuration options for Istio telemetry settings, including the addition of new fields in the Changes
Poem
Tip OpenAI O1 model for chat
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- .gitignore (1 hunks)
- README.md (1 hunks)
- api/v1alpha1/application_types.go (3 hunks)
- api/v1alpha1/istiotypes/istio_settings.go (1 hunks)
- api/v1alpha1/istiotypes/zz_generated.deepcopy.go (1 hunks)
- api/v1alpha1/skipobj_interfaces.go (2 hunks)
- api/v1alpha1/zz_generated.deepcopy.go (2 hunks)
- config/crd/skiperator.kartverket.no_applications.yaml (1 hunks)
- config/rbac/role.yaml (1 hunks)
- go.mod (4 hunks)
- internal/controllers/application.go (5 hunks)
- pkg/resourcegenerator/istio/telemetry/telemetry.go (1 hunks)
- pkg/resourceschemas/schemas.go (3 hunks)
- pkg/util/constants.go (1 hunks)
- tests/application/telemetry/application-assert.yaml (1 hunks)
- tests/application/telemetry/application-custom-tracing-assert.yaml (1 hunks)
- tests/application/telemetry/application-custom-tracing.yaml (1 hunks)
- tests/application/telemetry/application.yaml (1 hunks)
- tests/application/telemetry/chainsaw-test.yaml (1 hunks)
Files skipped from review due to trivial changes (3)
- api/v1alpha1/istiotypes/zz_generated.deepcopy.go
- tests/application/telemetry/application-custom-tracing-assert.yaml
- tests/application/telemetry/application.yaml
Additional comments not posted (31)
.gitignore (1)
10-11: LGTM!The changes to the
.gitignorefile look good:
- The new pattern
istio-*aligns with the common naming convention for Istio-related files and directories.- The negation pattern
!**/istio/ensures that theistiodirectory itself is not ignored.tests/application/telemetry/application-assert.yaml (1)
1-9: LGTM!The Telemetry resource definition looks good:
- It uses the correct API version and kind.
- The tracing configuration is valid, with a provider name and sampling percentage specified.
- The file aligns with the PR objective of creating telemetry resources by default for applications.
The code changes are approved.
tests/application/telemetry/application-custom-tracing.yaml (1)
10-11: LGTM!The custom tracing settings are correctly configured.
tests/application/telemetry/chainsaw-test.yaml (1)
1-19: LGTM!The test case is well-structured and follows the Chainsaw testing framework conventions. It tests the creation and application of resources related to the application and custom tracing, which is relevant to the changes introduced in this PR.
The test case is not skipped and is set to run concurrently, which is good for testing the changes in a realistic environment. The test case is also not set to skip the deletion of resources after the test run, which ensures that the resources are cleaned up after the test run.
The test case is using YAML files for resource creation and assertion, which is a common practice in Kubernetes-based testing. This makes the test case readable and maintainable.
Overall, the test case is a good addition to the test suite and helps ensure the correctness of the changes introduced in this PR.
api/v1alpha1/skipobj_interfaces.go (4)
5-5: LGTM!The code changes are approved.
22-22: LGTM!The code changes are approved.
23-23: LGTM!The code changes are approved.
24-24: LGTM!The code changes are approved.
pkg/util/constants.go (1)
14-15: LGTM! Please verify the usage of the new constant.The new constant
IstioTraceProvideris correctly defined and the comment provides a clear description of its purpose.However, without additional context on how this constant is used, it's difficult to provide more specific feedback. Please ensure that the constant is being used correctly and consistently throughout the codebase.
To verify the constant usage, run the following script:
Verification successful
Constant
IstioTraceProvideris used correctly and consistently.The constant
IstioTraceProvideris defined inpkg/util/constants.goand is used inpkg/resourcegenerator/istio/telemetry/telemetry.goin a manner consistent with its intended purpose as a trace provider name. No issues were found with its usage.
pkg/resourcegenerator/istio/telemetry/telemetry.go: Used in telemetry configuration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the constant `IstioTraceProvider`. # Test: Search for the constant usage. Expect: Consistent usage throughout the codebase. rg --type go -A 5 $'IstioTraceProvider'Length of output: 1343
api/v1alpha1/istiotypes/istio_settings.go (4)
1-11: LGTM!The package declaration and comments are approved.
12-17: LGTM!The
Tracingstruct is well-defined with appropriate field type and annotations.
22-25: LGTM!The
Telemetrystruct is well-defined with appropriate field type and annotation.
30-33: LGTM!The
IstioSettingsstruct is well-defined with appropriate field type and annotation.pkg/resourcegenerator/istio/telemetry/telemetry.go (1)
13-55: LGTM!The code changes are approved.
config/rbac/role.yaml (1)
194-205: LGTM!The new RBAC rules for managing
telemetriesresources in thetelemetry.istio.ioAPI group look good. The permissions are appropriate and follow the principle of least privilege by only allowing the necessary verbs. This is consistent with the PR objective of creating telemetry resources by default for applications.pkg/resourceschemas/schemas.go (3)
15-15: LGTM!The new import statement is necessary to include telemetry schemas into the existing runtime scheme.
36-36: LGTM!The change ensures that telemetry schemas are registered with the provided runtime scheme.
66-66: LGTM!The change integrates telemetry resources into the application's schema retrieval process.
README.md (2)
166-171: LGTM!The new
podSettingssection allows customizing the Pod Template used by Skiperator to create Deployments. The settings are optional, well-documented, and have sensible defaults.
172-177: LGTM!The new
istioSettingssection allows configuring Istio-specific resources, currently only adjusting the sampling interval for tracing. The settings are optional, well-documented, and have sensible defaults.go.mod (2)
10-10: LGTM!The changes to the
github.com/golang/protobufdependency are approved. The shift from an indirect to a direct dependency on the same version indicates a more explicit usage of the package in the codebase.
21-21: LGTM!The changes to the
google.golang.org/protobufdependency are approved. The shift from an indirect to a direct dependency on the same version indicates a more explicit usage of the package in the codebase.api/v1alpha1/application_types.go (3)
7-7: LGTM!The import statement is necessary to use the
istiotypes.IstioSettingstype.
247-251: LGTM!The
IstioSettingsfield is correctly defined in theApplicationSpecstruct.
437-439: LGTM!The
GetCommonSpecmethod is correctly updated to include theIstioSettingsfield.internal/controllers/application.go (4)
20-20: LGTM!The import statement for the telemetry package is necessary to utilize telemetry-related resources in the application reconciler.
62-62: LGTM!The RBAC annotation is necessary to allow the reconciler to perform operations such as get, list, watch, create, update, and delete on the
telemetriesresource group.
90-90: LGTM!The
Ownsstatement is necessary to indicate that the reconciler will manage instances of theTelemetryresource type.
202-202: LGTM!The
telemetry.Generatefunction call is necessary to ensure that telemetry resources are processed during the reconciliation loop.api/v1alpha1/zz_generated.deepcopy.go (1)
207-211: LGTM!The code changes for deep copying the
IstioSettingsfield are implemented correctly.config/crd/skiperator.kartverket.no_applications.yaml (1)
652-679: LGTM!The new
istioSettingsproperty provides a structured way to configure Istio telemetry settings, particularly for tracing. TherandomSamplingPercentageproperty allows adjusting the sampling interval for tracing.
|
LGTM |
|
|
||
| istioSettings := object.GetCommonSpec().IstioSettings | ||
|
|
||
| // TODO: Should we add KindPostFixedName() for the SKIPObject interface and use that for the telemetry name to avoid conflicts? |
There was a problem hiding this comment.
maybe we can override GetName? I don't want to add a function in the interface for only one object
There was a problem hiding this comment.
Wouldn't a hash of namespace + name be sufficient as far as uniqueness goes? See the util package.
There was a problem hiding this comment.
Would in that case be a hash of namespace + name + kind as the problem I want to avoid is creating an application and skipjob with the same name.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
tests/skipjob/telemetry/skipjob.yaml (1)
12-12: Add a newline at the end of the file.The yamllint tool indicates that there is no newline character at the end of the file.
While not a critical issue, adding a newline at the end of the file is considered a best practice and improves consistency.
Add a newline at the end of the file:
print bpi(2000)" +Tools
yamllint
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/telemetry/application-assert.yaml (1)
1-16: LGTM! The YAML configuration for the Telemetry resource looks good.The configuration follows the expected structure and conventions for defining a Telemetry resource in Istio. The tracing settings and label selectors are properly specified.
Consider adding a comment to clarify the purpose of the
($namespace)placeholder in the label selector. For example:application.skiperator.no/app-namespace: ($namespace) # Placeholder for dynamic namespace valueThis will help improve the readability and maintainability of the configuration.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/v1alpha1/application_types.go (3 hunks)
- api/v1alpha1/istiotypes/istio_settings.go (1 hunks)
- api/v1alpha1/istiotypes/zz_generated.deepcopy.go (1 hunks)
- api/v1alpha1/skipjob_types.go (3 hunks)
- api/v1alpha1/zz_generated.deepcopy.go (3 hunks)
- config/crd/skiperator.kartverket.no_applications.yaml (1 hunks)
- config/crd/skiperator.kartverket.no_skipjobs.yaml (2 hunks)
- config/rbac/role.yaml (1 hunks)
- go.mod (4 hunks)
- internal/controllers/skipjob.go (4 hunks)
- pkg/resourcegenerator/istio/telemetry/telemetry.go (1 hunks)
- tests/application/telemetry/application-assert.yaml (1 hunks)
- tests/application/telemetry/application-custom-tracing-assert.yaml (1 hunks)
- tests/application/telemetry/chainsaw-test.yaml (1 hunks)
- tests/skipjob/telemetry/chainsaw-test.yaml (1 hunks)
- tests/skipjob/telemetry/skipjob-assert.yaml (1 hunks)
- tests/skipjob/telemetry/skipjob-custom-tracing-assert.yaml (1 hunks)
- tests/skipjob/telemetry/skipjob-custom-tracing.yaml (1 hunks)
- tests/skipjob/telemetry/skipjob.yaml (1 hunks)
Files skipped from review due to trivial changes (4)
- api/v1alpha1/istiotypes/zz_generated.deepcopy.go
- tests/skipjob/telemetry/skipjob-assert.yaml
- tests/skipjob/telemetry/skipjob-custom-tracing-assert.yaml
- tests/skipjob/telemetry/skipjob-custom-tracing.yaml
Additional context used
yamllint
tests/skipjob/telemetry/skipjob.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (27)
tests/skipjob/telemetry/skipjob.yaml (1)
1-12: LGTM!The SKIPJob resource definition looks good:
- It follows the correct structure for a SKIPJob.
- The container image and command are valid.
- The resource definition is minimal and focused on a specific task.
The code changes are approved.
Tools
yamllint
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
tests/skipjob/telemetry/chainsaw-test.yaml (2)
1-19: LGTM!The YAML configuration for the Chainsaw test looks good. The test is well-structured and follows the expected format.
12-12: Verify the referenced files.The test references the following files:
skipjob.yamlskipjob-assert.yamlskipjob-custom-tracing.yamlskipjob-custom-tracing-assert.yamlPlease ensure that these files exist and contain the expected content for testing the telemetry functionality.
Also applies to: 14-14, 17-17, 19-19
tests/application/telemetry/chainsaw-test.yaml (1)
1-20: LGTM!The test case configuration looks good:
- The YAML structure and syntax are valid.
- The
apiVersionandkindfields correctly identify the resource type.- The
metadata.namefield provides a descriptive name for the test case.- The
spec.skip,spec.concurrent, andspec.skipDeletefields are appropriately set based on the desired test behavior.- The test steps are well-organized, with each step containing a try block with create/apply and assert actions.
- The referenced files for create/apply and assert actions provide the necessary resources and assertions for the test case.
The test case is ready to be added to the test suite.
tests/application/telemetry/application-custom-tracing-assert.yaml (1)
1-16: LGTM!The Telemetry resource configuration looks good:
- Correctly uses the
telemetry.istio.io/v1API version.- Properly structures the metadata and spec fields.
- Includes a valid tracing setup with a custom provider and sampling percentage.
- Uses appropriate labels in the selector to target the intended application.
No issues or improvements identified.
api/v1alpha1/istiotypes/istio_settings.go (4)
16-16: The existing comments have already discussed the field name and type. The current implementation using an integer percentage field seems sufficient based on the developer's response.
12-17: LGTM!The
Tracingstruct and itsRandomSamplingPercentagefield are properly defined with appropriate validation and default value.
22-26: The existing comments have already discussed the field type. Using a slice of pointers[]*Tracingis the correct type based on the developer's response.
31-35: LGTM!The
IstioSettingsstruct and itsTelemetryfield are properly defined with appropriate validation and default value.pkg/resourcegenerator/istio/telemetry/telemetry.go (1)
14-54: LGTM!The code changes are approved.
config/rbac/role.yaml (1)
194-205: LGTM!The changes expand the skiperator ClusterRole's permissions to manage telemetry resources in the
telemetry.istio.ioAPI group. The added permissions allow create, delete, get, list, patch, update, and watch operations on thetelemetriesresource, which seem appropriate for the skiperator controller.The changes are consistent with the AI-generated summary and align with the PR objective of creating telemetry resources by default for applications.
api/v1alpha1/skipjob_types.go (3)
6-6: LGTM!The import statement is necessary to use the
IstioSettingstype.
90-90: LGTM!The
IstioSettingsfield is correctly defined in theSKIPJobSpecstruct.
- It is marked as optional with a default value, which is a good practice.
- The default value for the telemetry tracing sampling percentage is set to a reasonable value of 10.
290-292: LGTM!The changes to the
GetCommonSpecmethod are correct.
- The
IstioSettingsfield is included in the returnedCommonSpecobject, ensuring that the common specification reflects the new configuration option.- The field is correctly accessed from the
skipJob.Specstruct.go.mod (2)
10-10: Approve addition of direct dependency ongithub.com/golang/protobuf.The addition of a direct dependency on
github.com/golang/protobufat versionv1.5.4is approved.However, please verify that changing this from an indirect to a direct dependency is intentional and consider any potential implications of this shift.
20-20: Approve addition of direct dependency ongoogle.golang.org/protobuf.The addition of a direct dependency on
google.golang.org/protobufat versionv1.34.2is approved.However, please verify that changing this from an indirect to a direct dependency is intentional and consider any potential implications of this shift.
internal/controllers/skipjob.go (5)
12-12: LGTM!The import statement for the telemetry package is necessary for integrating telemetry functionalities.
42-42: LGTM!The RBAC annotation for telemetry resources is necessary for the controller to manage telemetry objects.
80-80: LGTM!The ownership of the Telemetry resource is necessary for the controller to monitor and manage telemetry objects.
153-153: LGTM!The call to telemetry.Generate is necessary for generating telemetry resources as part of the reconciliation process.
Line range hint
1-345: SkippedThe remaining code changes are not directly related to the telemetry integration and do not require additional review comments.
api/v1alpha1/application_types.go (2)
247-252: LGTM!The addition of the
IstioSettingsfield to theApplicationSpecstruct is approved. It allows for the configuration of Istio-specific resources and enhances the application's capabilities in managing telemetry settings. The field is correctly marked as optional and a sensible default value is provided.
438-440: LGTM!The changes to the
GetCommonSpecmethod are approved. The inclusion of theIstioSettingsfield when returning aCommonSpecinstance ensures that the new configuration is accessible through this method. The changes are consistent with the addition of theIstioSettingsfield to theApplicationSpecstruct.api/v1alpha1/zz_generated.deepcopy.go (2)
207-211: LGTM!The changes to the
DeepCopyIntomethod for theApplicationSpecstruct look good. TheIstioSettingsfield is correctly handled by checking if it's not nil, creating a new instance ofistiotypes.IstioSettings, and deep copying the existingIstioSettingsinto the new instance.
611-615: LGTM!The changes to the
DeepCopyIntomethod for theSKIPJobSpecstruct look good. Similar to the changes made in theDeepCopyIntomethod for theApplicationSpecstruct, theIstioSettingsfield is correctly handled by checking if it's not nil, creating a new instance ofistiotypes.IstioSettings, and deep copying the existingIstioSettingsinto the new instance.config/crd/skiperator.kartverket.no_skipjobs.yaml (1)
786-820: LGTM!The new
istioSettingsfield in theSKIPJobCRD spec looks good:
- It allows configuring Istio-specific telemetry settings, particularly the sampling interval for tracing.
- The field and its nested properties are well-documented.
- It has a valid default configuration for telemetry tracing.
- Using a simple integer type for
randomSamplingPercentageis justified due to compatibility with the code generator.- It is appropriately marked as a required field.
The changes are approved.
config/crd/skiperator.kartverket.no_applications.yaml (1)
652-686: LGTM!The new
istioSettingsconfiguration section and its properties are well-structured and provide useful options for configuring Istio telemetry settings. The design decision to use a simple integer forrandomSamplingPercentageis justified and the value constraints are appropriate.The code changes are approved.
|
|
||
| istioSettings := object.GetCommonSpec().IstioSettings | ||
|
|
||
| // TODO: Should we add KindPostFixedName() for the SKIPObject interface and use that for the telemetry name to avoid conflicts? |
There was a problem hiding this comment.
Wouldn't a hash of namespace + name be sufficient as far as uniqueness goes? See the util package.
Summary by CodeRabbit
New Features
telemetryv1API from Istio, enabling handling of telemetry resources in the application.istioSettingsin job specifications, detailing telemetry settings for better control over tracing behavior.Bug Fixes
Documentation
Chores