Skip to content

Add ServiceStats to Service#562

Merged
wesleyjellis merged 3 commits into
mainfrom
wesley/service-stats
Jun 6, 2025
Merged

Add ServiceStats to Service#562
wesleyjellis merged 3 commits into
mainfrom
wesley/service-stats

Conversation

@wesleyjellis
Copy link
Copy Markdown
Contributor

Resolves https://gitlab.com/jklabsinc/OpsLevel/-/work_items/13509

Problem

We don't support service stats and rubric the API

Solution

Add those things with the new Service.GetServiceStats method. It's a beefy query so I didn't want to fetch it by default.

I did have to add CheckId to avoid loops where levels have checks which have levels which have checks which have levels which have checks....

PR to client-gen goes with this too, incoming shortly

Checklist

  • I have run this code, and it appears to resolve the stated issue.
  • This PR does not reduce total test coverage
  • This PR has no user interface changes or has already received approval from product management to change the interface.
  • Does this change require a Terraform schema change?
    • If so what is the ticket or PR #
  • Make a changie entry that explains the customer facing outcome of this change

Add GetServiceStats to Service, which returns Rubric and the checks /
levels associated with that for a given service.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new ServiceStats feature to the Service API, which includes retrieving rubric information and check results through an updated GetServiceStats method. Key changes include modifying various query templates and test cases to include the new "checks" field on level objects, updating the underlying types (such as replacing Check with CheckId), and adding new connection types and structs to support service stats.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

File Description
testdata/templates/*.tpl, system_test.go, service_test.go, etc. Updated GraphQL queries to include the new "checks" field in level fragments.
service.go Added GetServiceStats method with validation for service id and proper GraphQL query handling.
object.go, check.go, connection.go, maturity_test.go, level_test.go Updated type definitions and connections to support new schema fields.
.changes/unreleased/Feature-20250606-102556.yaml Added changelog entry for ServiceStats feature.
Comments suppressed due to low confidence (3)

service.go:359

  • The error message in GetServiceStats reports an invalid service id by printing an empty string. Consider whether including the service id here adds meaningful context or if a static message would be preferable, and ensure consistency with other error messages in the project.
return nil, fmt.Errorf("unable to get 'ServiceStats', invalid service id: '%s'", service.Id)

object.go:313

  • Ensure that the new 'checks' field added to Level is clearly documented in the code and corresponding API documentation, as it now provides minimal check information (CheckId) instead of the full Check object.
"Checks      []CheckId // The checks that belong to the level (Optional)"

testdata/templates/scorecards.tpl:2

  • Confirm that the updated GraphQL mutation in the scorecard templates—including the new 'checks' field in level objects—aligns with the expected response structure and is fully covered by tests.
"+mutation ScorecardCreate($input:ScorecardInput!){scorecardCreate(input: $input){scorecard{...servicesReport{levelCounts{level{alias,checks{id,name},description,id,index,name},serviceCount}},...}"

Comment thread .changes/unreleased/Feature-20250606-102556.yaml Outdated
Copy link
Copy Markdown
Contributor

@rocktavious rocktavious left a comment

Choose a reason for hiding this comment

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

Nice clean work. Only suggestion is to beef up the changie entry to explain the change better to customers.

There is a slight backwards incompatible change with changing of CheckResult to use CheckId ... but i'm not sure of the implications as i don't think it used in the CLI or Terraform provider. Would you mind doing a quick scan to see? If there is usage there we might want to add a changie entry for the backwards in-compatible change as someone use the CLI might expect the extra data of type Check to be there and now they just get the Id and Name.

Co-authored-by: Kyle <kyle@opslevel.com>
@wesleyjellis
Copy link
Copy Markdown
Contributor Author

@rocktavious, both terraform and the cli compile and test with this commit (after fixing 1 think in CLI unrelated to this) and I couldn't find any references to CheckLevel in their usage.

Hadn't noticed it was a breaking change, but you're right, I'll add a changie entry too

@rocktavious
Copy link
Copy Markdown
Contributor

@wesleyjellis ya i'm pretty certain it won't affect terraform - but the CLI does alot of "implict" stuff where if the struct is part of some other struct as a nested field - now the CLI is missing output data that someone might be depending on.

Replacing Check with CheckId on CheckResult is a breaking change, add a
note about that
@wesleyjellis wesleyjellis force-pushed the wesley/service-stats branch from 90c5cc1 to 913674f Compare June 6, 2025 19:15
@wesleyjellis
Copy link
Copy Markdown
Contributor Author

wesleyjellis commented Jun 6, 2025

I'm confident it should be fine after looking again. CheckResult is only accessible via CheckResultsConnection and ServiceMaturityReport. CheckResultsConnection is newly added and there doesn't appear to be any way to query/use ServiceMaturityReport

@wesleyjellis wesleyjellis merged commit 1ccf047 into main Jun 6, 2025
6 checks passed
@wesleyjellis wesleyjellis deleted the wesley/service-stats branch June 6, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants