Skip to content

ROSAENG-14107: new 'hcp-dashboard' sub-command for 'osdctl rhobs' #920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nikokolas3270 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ROSAENG-14107: new 'hcp-dashboard' sub-command for 'osdctl rhobs' #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions cmd/rhobs/hcpDashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package rhobs

import (
"fmt"
"strings"

"github.com/pkg/browser"
"github.com/spf13/cobra"
)

func newCmdHcpDashboard() *cobra.Command {
var rhobsCell string
var isOpeningGrafanaUrl bool
var dashboardName string

cmd := &cobra.Command{
Use: "hcp-dashboard [dashboard-name]",
Short: "Get the HCP dashboard URL for a given HCP cluster",
Long: "Get the HCP dashboard URL for a given HCP cluster. " +
"The dashboard name is optional and defaults to the hosted cluster dashboard. " +
"Allowed values for the dashboard name are: " + strings.Join(GetAllowedGrafanaDashboardsShortNames(), ", ") + ". " +
"The URL of the RHOBS cell(s) to use can be specified with the --rhobs-cell option, " +
"but it is usually more convenient to specify the cluster with the --cluster-id option and let the command figure out the RHOBS cell(s) to use. " +
"Note that the --rhobs-cell option is not working with all dashboards and cannot be used together with the --cluster-id option.",
Args: cobra.MaximumNArgs(1),
SilenceErrors: true,
RunE: func(cmd *cobra.Command, args []string) error {
dashboardName = defaultGrafanaDashboardShortName
if len(args) == 1 {
dashboardName = args[0]
}

grafanaDashboard := GetGrafanaDashboardForShortName(dashboardName)
if grafanaDashboard == nil {
return fmt.Errorf("invalid dashboard name: %s", dashboardName)
}

rhobsCells := []string{}
if rhobsCell != "" {
if cmd.Parent().Flags().Changed("cluster-id") {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return fmt.Errorf("--rhobs-cell and --cluster-id options cannot be used together")
}

rhobsCells = strings.Split(rhobsCell, ",")
if len(rhobsCells) > 2 {
return fmt.Errorf("value passed to --rhobs-cell must have at most 2 elements: %s", rhobsCell)
}
}

cmd.SilenceUsage = true

var metricsRhobsFetcher, logsRhobsFetcher *RhobsFetcher
var err error

if len(rhobsCells) > 0 {
metricsRhobsFetcher, err = CreateRhobsFetcherFromCell(rhobsCells[0])
if err != nil {
return err
}
if len(rhobsCells) == 2 {
logsRhobsFetcher, err = CreateRhobsFetcherFromCell(rhobsCells[1])
if err != nil {
return err
}
} else {
logsRhobsFetcher = metricsRhobsFetcher
}
} else {
metricsRhobsFetcher, err = CreateRhobsFetcher(commonOptions.clusterId, RhobsFetchForMetrics, commonOptions.hiveOcmUrl)
if err != nil {
return err
}
logsRhobsFetcher, err = CreateRhobsFetcher(commonOptions.clusterId, RhobsFetchForLogs, commonOptions.hiveOcmUrl)
if err != nil {
return err
}
}
Comment on lines +69 to +77

@coderabbitai coderabbitai Bot Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if CreateRhobsFetcher performs network calls or expensive operations

# Find the implementation
ast-grep --pattern $'func CreateRhobsFetcher($$$) $$$'

# Look for HTTP calls, OCM client usage, or heavy operations
rg -n -C5 'func CreateRhobsFetcher|http\.|ocm\.|Get.*Cluster' --type=go -g '**/rhobs/**'

Repository: openshift/osdctl

Length of output: 11631

Avoid redundant OCM/cluster/config initialization when creating metrics vs logs RHOBS fetchers

cmd/rhobs/hcpDashboard.go creates two RhobsFetchers back-to-back (metrics + logs) for the same commonOptions.clusterId, and each call to CreateRhobsFetcher performs expensive setup: it creates a new OCM connection, resolves the same cluster (and hypershift management cluster when applicable), resolves the RHOBS cell, and calls osdctlConfig.EnsureConfigFile() on every invocation. This duplicates network/IO work and config reload—refactor to share/memoize the common parts between the two fetchers.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/rhobs/hcpDashboard.go` around lines 69 - 77, The code redundantly calls
CreateRhobsFetcher twice causing duplicate OCM/cluster/cell resolution and
repeated osdctlConfig.EnsureConfigFile() work; refactor by extracting the shared
initialization into a new helper (e.g., CreateRhobsFetcherContext or
InitRhobsFetchDependencies) that performs OCM connection, cluster/hypershift
resolution, RHOBS cell lookup, and ensures the osdctl config once, and then
change CreateRhobsFetcher (or add NewRhobsFetcherFromContext) to accept that
shared context and only create the specific RhobsFetcher for
RhobsFetchForMetrics and RhobsFetchForLogs; update the call site in
hcpDashboard.go to call the init helper once, then create both metrics and logs
fetchers from the returned context.

@Nikokolas3270 Nikokolas3270 Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe there should be a CreateRhobsFetchers method to mutualise those accesses.
Original code was created with the idea that a cluster has only one rhobs cell... But in reality the one used for logs can differ from the one for metrics

@coderabbitai coderabbitai Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look at the CreateRhobsFetcher implementation to understand shared vs diverging parts
ast-grep --pattern $'func CreateRhobsFetcher($$$) ($$$) {
  $$$
}'

# Also check RhobsFetcher struct definition
ast-grep --pattern $'type RhobsFetcher struct {
  $$$
}'

# Check what RhobsFetchForMetrics / RhobsFetchForLogs diverge on
rg -n 'RhobsFetchFor|fetchType|FetchType|metricsCell|logsCell|IsHostedCluster' --type=go

Length of output: 6218

@Nikokolas3270, that context is helpful — the design makes sense since getRhobsCell diverges per usage (metrics vs logs) but everything upstream of it (OCM connection, cluster resolution, MC lookup, config reload) is purely shared work.

A CreateRhobsFetchers function would fit naturally: do all the common init once, call getRhobsCell twice (once for each usage), then return both fetchers. Rough sketch:

// CreateRhobsFetchers creates both a metrics and a logs RhobsFetcher for the
// given cluster, sharing the OCM connection and cluster-resolution work.
func CreateRhobsFetchers(clusterKey string, hiveOcmUrl string) (metricsFetcher, logsFetcher *RhobsFetcher, err error) {
    ocmConn, err := ocmutils.CreateConnection()
    if err != nil {
        return nil, nil, err
    }
    defer ocmConn.Close()

    cluster, err := ocmutils.GetCluster(ocmConn, clusterKey)
    if err != nil {
        return nil, nil, err
    }

    var monitoredClusterId string
    isHcp := cluster.Hypershift().Enabled()
    isMC := false

    if isHcp {
        managementCluster, err := ocmutils.GetManagementCluster(cluster.ID())
        if err != nil {
            return nil, nil, fmt.Errorf("failed to retrieve management cluster for cluster '%s': %v", cluster.ID(), err)
        }
        monitoredClusterId = managementCluster.ID()
    } else {
        monitoredClusterId = cluster.ID()
        isMC, err = ocmutils.IsManagementCluster(cluster.ID())
        if err != nil {
            return nil, nil, fmt.Errorf("failed to determine if cluster '%s' is a management cluster: %v", cluster.ID(), err)
        }
    }

    // Cells CAN differ between metrics and logs — resolve both.
    metricsCell, err := getRhobsCell(monitoredClusterId, ocmConn, RhobsFetchForMetrics, hiveOcmUrl)
    if err != nil {
        return nil, nil, err
    }
    logsCell, err := getRhobsCell(monitoredClusterId, ocmConn, RhobsFetchForLogs, hiveOcmUrl)
    if err != nil {
        return nil, nil, err
    }

    // Reload config once after backplane may have mutated it.
    if err = osdctlConfig.EnsureConfigFile(); err != nil {
        return nil, nil, fmt.Errorf("failed to reload osdctl config: %v", err)
    }

    base := RhobsFetcher{
        clusterId:           cluster.ID(),
        clusterExternalId:   cluster.ExternalID(),
        clusterName:         cluster.Name(),
        IsHostedCluster:     isHcp,
        isManagementCluster: isMC,
        ocmEnvName:          ocmutils.GetCurrentOCMEnv(ocmConn),
    }
    mf := base
    mf.RhobsCell = metricsCell
    lf := base
    lf.RhobsCell = logsCell
    return &mf, &lf, nil
}

hcpDashboard.go lines 69-77 and cellCmd.go lines 20-25 both exhibit the same pattern, so both call sites would benefit from switching to this helper.

Would you like me to open a follow-up GitHub issue to track this refactor, or plan the code changes directly?

✏️ Learnings added 
Learnt from: Nikokolas3270
Repo: openshift/osdctl PR: 920
File: cmd/rhobs/hcpDashboard.go:69-77
Timestamp: 2026-06-12T15:13:45.487Z
Learning: In openshift/osdctl cmd/rhobs, the metrics RHOBS cell and logs RHOBS cell for a given cluster can be different. Any helper that creates both a metrics and a logs RhobsFetcher (e.g., a future CreateRhobsFetchers) must call getRhobsCell separately for RhobsFetchForMetrics and RhobsFetchForLogs, even though all other initialization (OCM connection, cluster resolution, management cluster lookup, EnsureConfigFile) is shared and should be done only once.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: Nikokolas3270
Repo: openshift/osdctl PR: 920
File: cmd/rhobs/hcpDashboard.go:40-40
Timestamp: 2026-06-12T15:09:42.380Z
Learning: When implementing osdctl Cobra command logic in cmd/**/*.go, do not infer whether the user explicitly set a CLI flag by checking the derived/resolved option value (e.g., commonOptions.clusterId), especially if a parent command pre-invoke hook auto-populates that value even when the flag is omitted. Instead, detect explicit flag usage via Cobra’s flag API, e.g. use `cmd.Parent().Flags().Changed("cluster-id")` (or `cmd.Flags().Changed(...)` as appropriate for the flag being checked) to determine whether `--cluster-id` was explicitly provided by the user.


grafanaUrl, err := GetGrafanaDashboardUrl(metricsRhobsFetcher, logsRhobsFetcher, grafanaDashboard)
if err != nil {
return fmt.Errorf("failed to compute Grafana URL: %v", err)
}
if isOpeningGrafanaUrl {
err = browser.OpenURL(grafanaUrl)
if err != nil {
return fmt.Errorf("failed to open Grafana URL in browser: %v", err)
}
} else {
fmt.Println(grafanaUrl)
}

return nil
},
}

cmd.Flags().StringVarP(&rhobsCell, "rhobs-cell", "c", "", "RHOBS cell URL - "+
"for instance: https://us-east-1-0.rhobs.api.stage.openshift.com - "+
"use a comma to separate the RHOBS cell to use for metrics from the logs RHOBS cell if they are different - "+
"this option is not working with all dashboards - exclusive with --cluster-id")
cmd.Flags().BoolVarP(&isOpeningGrafanaUrl, "browser", "b", false, "Open in the URL in the default browser")

return cmd
}
Loading