enhancements: add SEP for metrics integration#2028
Conversation
Signed-off-by: Mayleigh Tjapkes <mamyers@redhat.com>
|
Hi @mayleighnmyers. Thanks for your PR. I'm waiting for a istio-ecosystem or istio member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
|
||
| A broader Observability Integration API with target references to a monitoring and tracing stacks is being discussed in the initial PR comments. We plan to design and implement the monitoring controller and broader Integration API controller separately. This enhancement proposal is focused on the monitoring controller with API change in the `Istio` Custom Resource. We will work toward a broader Integration API design and implementation in next phase. | ||
|
|
||
| ### Architecture |
There was a problem hiding this comment.
How will deletion be handled? I think it worth to mention how we plan to hanlde the deletion of the resources
There was a problem hiding this comment.
There is a concern from existing environments where users have configured their custom ServiceMonitor or PodMontior resources manually. They expect those existing resources should not be deleted from Sail Operator.
We will add a paragraph about deletion in SEP. Thanks.
There was a problem hiding this comment.
Checking ownership can help us to solve this? we can add specific metadata to allow us to easily filter which resources are handle by our operator
There was a problem hiding this comment.
yes, that helps. Checking labels should also work when the monitoring controller creates those resources with a unique label such as managed-by: mesh-operator
Signed-off-by: Mayleigh Tjapkes <mamyers@redhat.com>
|
/retest |
|
|
||
| ## Implementation Plan | ||
|
|
||
| v1alpha1 |
There was a problem hiding this comment.
what does v1alpha1 mean in this case?
There was a problem hiding this comment.
It refers to API changes we will introduce for enabling this controller. The initial plan was simply adding a spec.monitoring.enabled field in the existing Istio CRD so we don't need this v1alpha1 .
Nick suggested an alternative new API , see his comment in :
#1959 (comment)
If we will introduce a new API CRD such as Integration , then we need to put it in a v1alpha1 version.
We described this alternative API in the section "Alternatives API Considered" above.
That approach will introduce additional discussions about API changes and more general integration controller(s). I feel those need to be in follow up SEPs and this SEP focuses on metric monitoring controller first.
I will remove this v1alpha1 here. Thanks.
There was a problem hiding this comment.
just a nit on the filename: we have migrated to numbering the SEPs based on the issue that they're about. it appears we don't have an open issue that tracks this work, so that would be the first step, and then we can call this SEP12345 (if 12345 ends up being the issue number)
There was a problem hiding this comment.
I see. This was requested in a JIRA by Nick and we have an epic , tasks. I will update this file name with the JIRA number.
yxun
changed the title
Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>
Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>
yxun
changed the title
Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>
yxun
force-pushed
the
enhancements/sep3-observability
branch
from
4d84c44 to
062add7
Compare
|
/test integ-tests-arm64 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
==========================================
+ Coverage 76.32% 76.75% +0.42%
==========================================
Files 58 58
Lines 3181 3222 +41
==========================================
+ Hits 2428 2473 +45
+ Misses 612 610 -2
+ Partials 141 139 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Related Issue/PR #
#1959
Additional information: