che #17186 Enabling metrics by default

[ibuziuk]

eclipse-che/che#17186

CHE_METRICS_ENABLED will be set to true by default in the cm

list of metrics available in the Prometheus format - https://gist.github.com/ibuziuk/1859a134bb7575cecd7f7669100a14f4

[ibuziuk]

@davidfestal @tolusha will this change be enough to have metrics enabled in the default CR when installing via operatorHub?

[tolusha]

@ibuziuk
That's not enough. It is a bit tricky but not hard.

1. Update comment in che_types.go instead of org_v1_che_crd.yaml
2. Launch olm/update-cr-crd-files.sh to correctly update org_v1_che_crd.yaml
3. Launch olm/update-nightly-olm-files.sh to generate new nighly olm files
4. Set 'spec.metrics.enabled: true' in both (kubernetes and openshift) new clusterserviceversion files
5. Update both new (kubernetes and openshift) clusterserviceversion.yaml.diff files

Here is an example how we set tlsSupport: true #186

[ibuziuk]

@tolusha done. Could you please clarify how can I verify it via operatorhub ? maybe there are some docs for this?

[che-bot]

Latest version of Eclipse Che installed and tested successfully on minikube.

[tolusha]

@ibuziuk
Sounds ok. One more point, pls update clusterserviceversion.yaml.diff files (since we changed csv file manually):
diff -u <prev csv file> <new csv file>

This guide [1] help you to test your changes.
[1] https://github.com/eclipse/che-operator/blob/master/README.md#how-to-test-operator-via-olm

[tolusha]

BTW
I think step 3 isn't needed in your case

[ibuziuk]

Sounds ok. One more point, pls update clusterserviceversion.yaml.diff files

@tolusha done

[che-bot]

Latest version of Eclipse Che installed and tested successfully on minikube.

[ibuziuk]

verified manually:

[openshift-ci-robot]

@ibuziuk: PR needs rebase.

Instructions 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/test-infra repository.

[tolusha]

@ibuziuk
Fixing conflicts in nightly olm files means:

1. remove all generated nightly olm files
2. revert changes in package.yaml files
3. rebase on master
4. create new nightly olm files

[ibuziuk]

@tolusha it is just insane how intricate changing the default property value process is... Created eclipse-che/che#17338

[che-bot]

Latest version of Eclipse Che installed and tested successfully on minikube.

[davidfestal]

Sorry to come too late here, but I'm not sure this documentation change is correct.
The value you propose in the CSV ALMExamples field (that will then be provided by OperatorHub when clicking on Create CheCluster button is not the same thing as the default field value

The real default value is the value that will be seen by the Che operator controller when no metrics.enable field is set. And since this enable field is a GO boolean, according to typical GO json serialization rules, omitting it is the same as setting its value to false. So I assume that default value is still false in this sense.

Strictly speaking, if we want to always enable metrics by default even when a user provides its own CheCluster custom resource without the metrics.enable field, then we should probably either:

1. Deprecate the enable field and add a new disable boolean field that would be false by default (typical GO behavior)
2. Have the enable field be a pointer to a boolean, so that you can distinguish between the no value and false cases. However this would not be backward-compatible with the previous versions of the CRD, so I'd rather go for option 1.

I assume this can be done in a future PR.

cc @ibuziuk @tolusha @l0rd

[tolusha]

@davidfestal
I see you point.
What about setting it to true if metrics.enable field isn't explicitly defined in CR?

[ibuziuk]

why should we do this, can't we simply fall back on default CR as described in the docs PR - eclipse-che/che-docs#1366

[ibuziuk]

verified it manually and this works just fine - #322 (comment)

[tolusha]

I see.
chectl uses the deploy/crds/org_v1_che_cr.yaml CR. So, metrics will be enabled.

[davidfestal]

What about setting it to true if metrics.enable field isn't explicitly defined in CR?

if metrics.enable field isn't explicitly defined in CR, it is seen as false by the controller afaict. When not using a pointer you cannot distinguish between 'no explicitly defined' and 'defined to false'.