Create ServiceMonitor to monitor target allocator #2416
Conversation
changexd
changed the title
There was a problem hiding this comment.
I think this LGTM. @changexd are all of the open questions resolved?
|
Not yet, there are two questions waiting for confirmation, should I make the changes first and wait for reviews? edit: I've made some final changes, I think it is ready for review : ) |
| if params.OtelCol.Spec.TargetAllocator.Observability.Metrics.EnableMetrics && featuregate.PrometheusOperatorIsAvailable.IsEnabled() { | ||
| resourceFactories = append(resourceFactories, manifests.FactoryWithoutError(ServiceMonitor)) | ||
| } |
There was a problem hiding this comment.
Should we create an explicit convention for where we want these checks to live @jaronoff97 @pavolloffay? My impression was that they should be in the manifest generation functions, which should return nil if there's nothing to generate. But I looked at the same logic for the collector, and there the check is also done in the builder.
I don't think this question necessarily needs to block the PR, we can change it later. But I'd like to have clarity here.
There was a problem hiding this comment.
+1 on being consistent and putting this knowledge into the manifest functions.
The manifest generation functions gets manifests.Params and can make that decision and encapsulate when the objects should be generated.
There was a problem hiding this comment.
I was originally thinking the check would be done in the resource's builder (like where it is now) so it's easy to understand what should be appearing. This would make individual unit testing easier for each of the manifests as they don't need to check on featuregates, however checking if a feature is enabled to make the manifest should indeed happen within the manifest function. I think I would then recommend the following:
- featurgates belong in the builder for the resource
- fields of the resource controlling whether a manifest is created (hpa, ingress, etc.) should be handled in the manifest itself
I think regardless of what we choose, we should do that in a followup.
|
@changexd looks like approvals are looking good, when you get a chance could you resolve the conflicts so we can get this merged before the next release? |
|
@jaronoff97 I've resolved the conflicts in code, as for |
|
@changexd that's actually fine – it looks like we missed some api-docs changes in a recent version upgrade. I ran it off of main and there was a diff, so this resolves that 😅 |
|
thank you for your contribution :D 🎉 |
* add create service monitor to targetallocator * add chlog * add e2e test to ta service monitor * fix linting * make bundle * fix chlog * add a condition to avoid creating sm when running in unsupported modes * update bundle * move ta sm ports into ta definition * remove changes to config/manager * assert ta service monitor creation in e2e * enable prometheus feature in e2e test * change description of observability field * move ta observability check to build * add target allocator to builder test * fix lint * fix ta sm manifest test * remove changes to manager kustomization * make ta sm return directly * move ta service monitor test to dedicated tests * remove commands in e2e * use function to generate label for sm * ta sm labels in builder test * fix api.md * fix bundling * make api-docs again * fix kustomization manager * fix ta service account in test according to fixed issues
Description:
this resolves #2311
Link to tracking Issue:
#2311
Testing:
I've added checks to make sure collector gets the target-allocator jobs
side note
I basically followed implementation from #1874, however I've encountered couple issues I wasn't sure if these are expected or not, if these needed to be fixed, I'll open issues and am willing to fix it.
The observability params in collector is not validated, so I did not add it either, should we add validation to these params?
The label
app.kubernetes.io/instancefrom default service of target allocator is$namespace.$collectorNameinstead of$namespace.$targetAllocatorName, should we change this?The service port name from TA is specifically map from “targetallocation” to container port “http”, but the collector service itself maps “prometheus” to container port “prometheus”, is this intentional?, if not, should this be fixed?
After disabling observability, the manager doesn't remove the servicemonitor, only after deletion of otelcol resource, it is then removed, this happens to both collector and target allocator.