feat: support logging as ui-plugin #477
Conversation
|
Skipping CI for Draft Pull Request. |
bundle/manifests/observability-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
xperimental
changed the title
xperimental
force-pushed
the
logging-ui
branch
2 times, most recently
from
fa45ab6
to
a2bff9f
Compare
|
Cleaned up the PR to only contain the changes needed for the logging-ui. Are we ok with the API of UIPlugin as-is? If yes, I'd remove the draft status. |
xperimental
requested review from
jan--f and
slashpai
and removed request for
a team
There was a problem hiding this comment.
Tested on OpenShift and it works as expected 👍 However I had to manually add the "openshift.enabled"... I'm guessing that in the future there will be a version on OLM with this flag already enabled.
To test:
- Install loki-operator using the console UI
- Install cluster-logging operator using version in https://github.com/xperimental/cluster-logging-operator/tree/block-console-5.9
- Install observability-operator using this PR
- Edit CSV to add "--openshift.enabled"
- Create ClusterLogging resource
- Create ClusterLogForwarder resource
- Create LokiStack resource called "lokistack-dev"
- Create UIPlugin with type "logging" and pass the name "lokistack-dev"
pkg/controllers/uiplugin/logging.go
Outdated
| func marshalLoggingPluginConfig(cfg *uiv1alpha1.LoggingConfig) (string, error) { | ||
| if cfg.LogsLimit == 0 && cfg.Timeout == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| pluginCfg := struct { | ||
| LogsLimit int `yaml:"logsLimit"` | ||
| Timeout string `yaml:"timeout"` | ||
| }{ | ||
| LogsLimit: cfg.LogsLimit, | ||
| Timeout: cfg.Timeout, | ||
| } | ||
|
|
||
| buf := &bytes.Buffer{} | ||
| if err := yaml.NewEncoder(buf).Encode(pluginCfg); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return buf.String(), nil | ||
| } |
There was a problem hiding this comment.
Can we re-think this a little bit? I suggest to decouple from the logging-view-plugin's ConfigMap fields and use something because some might simply carry unwanted baggage (e.g. logLimit vs. logsLimit) which we would carry in our API too.
|
Some updates from the code-review comments. Not done with all the changes yet, though. Notably, the additional options for the logging-view-plugin and the |
xperimental
force-pushed
the
logging-ui
branch
4 times, most recently
from
3da8eb7
to
509b415
Compare
There was a problem hiding this comment.
Updated the PR for the code-review feedback again.
While testing for the timeout in the ConfigMap I noticed that a changing ConfigMap also mandates a redeploy of the Deployment, so I added code for hashing the ConfigMap's contents so that restarts happen automatically.
One open question for me is, whether the options passed to the logging-view-plugin need another "level of indentation", especially if we plan to increase the number of options from the two that are currently available.
To illustrate my point: Currently the configuration would look like this:
apiVersion: observability.openshift.io/v1alpha1
kind: UIPlugin
metadata:
name: ui-logging
spec:
type: Logging
logging:
lokiStack:
name: lokistack-dev
timeout: 3m
logsLimit: 1000And with another level of indentation/grouping it would look like this (think of frontend as a placeholder, I couldn't think of anything better so far):
apiVersion: observability.openshift.io/v1alpha1
kind: UIPlugin
metadata:
name: ui-logging
spec:
type: Logging
logging:
lokiStack:
name: lokistack-dev
frontend:
timeout: 3m
logsLimit: 1000
|
Two points of additional information:
|
| // +kubebuilder:validation:Required | ||
| LokiStack LokiStackReference `json:"lokiStack"` | ||
|
|
||
| // LogsLimit is the max number of entries returned for a query. |
There was a problem hiding this comment.
(nit) Can we document what the zero value means? Is it equivalent to no limits?
There was a problem hiding this comment.
I think the logging-view-plugin defines its own default, which is used when no limit is specified (= "when it is 0"). If I am interpreting this correctly there's a default of 100 for normal queries and 200 for tail.
|
Rebase completed ✅ Any feedback on the question whether we should put the options for the frontend in a separate struct? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoaoBraveCoding, periklis, simonpasquier, xperimental The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implementation for COO-147.