-
Notifications
You must be signed in to change notification settings - Fork 28
feat: Pull cost attribution labels for tenant #1544
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.
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
base: main
Are you sure you want to change the base?
Conversation
ee3f691 to
3c4d581
Compare
Following a similar path as limits to pull cost attribution labels for tenants. Adds a new internal library `cals/tenant.go` that provides an interface that wraps the `TenantProvider` client. Updates all of the wiring from Updated => Scraper to pass along the new cal client. Starts the process of using the library in `scraper.go`, which right now finds the cost attribution labels for a specific tenant. Returns an empty slice if nothing is found. This will be dependant upon the API changes being released to get cost attribution labels defined at the tenant level.
Add a `testCalTenants` interface to mock out the cost attribution label servce. This fixes the tests that were failing on the latest commits to main.
It didn't quite feel right to call the package `cals` but then refer to everything as tenants. This change makes it such that everything is cals, and that the provider is a TenantProvider. I believe this clearly communicates what the cals package is meant for.
785ebb1 to
10208df
Compare
There was a problem hiding this 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 support for pulling cost attribution labels from tenant configuration. The implementation introduces a new cals package that wraps the TenantProvider client to fetch cost attribution labels at the tenant level, following a similar pattern to the existing limits functionality.
Key changes:
- New
calspackage withCostAttributionLabelsclient for fetching tenant-specific cost attribution labels - Updated scraper to retrieve and log cost attribution labels during check execution
- Wiring changes throughout the codebase to pass the new CAL client from main through to scrapers
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cals/cals.go | New package implementing TenantProvider wrapper for cost attribution labels |
| internal/cals/cals_test.go | Unit tests for the new cost attribution labels functionality |
| cmd/synthetic-monitoring-agent/main.go | Initializes and passes CAL client to the updater |
| internal/checks/checks.go | Wires CAL client through updater to scrapers |
| internal/checks/checks_test.go | Updates test scraper factory signature |
| internal/scraper/scraper.go | Integrates CAL client, retrieves and logs labels during scraping |
| internal/scraper/scraper_test.go | Adds test mock for CAL interface |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I must have taken the wrong change when rebasing. This caused issues with linters as gocyclo may have been removed
The-9880
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits, LGTM!
| labelsLimiter LabelsLimiter, | ||
| telemeter *telemetry.Telemeter, | ||
| secretStore *secrets.TenantSecrets, | ||
| costAttributionLabels *cals.CostAttributionLabels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the interface here as well:
| costAttributionLabels *cals.CostAttributionLabels, | |
| costAttributionLabels TenantCals, |
| labelsLimiter LabelsLimiter, | ||
| telemeter *telemetry.Telemeter, | ||
| secretStore *secrets.TenantSecrets, | ||
| cals *cals.CostAttributionLabels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the interface here as well:
| cals *cals.CostAttributionLabels, | |
| cals TenantCals, |
| tcal := NewCostAttributionLabels(testcase.tenantProvider) | ||
| cals, err := tcal.CostAttributionLabels(context.Background(), 1) | ||
| if testcase.expectError { | ||
| require.NotNil(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're interested in a typed assertion here, you could define the following error in cals.go:
var (
ErrTenantProvider := errors.New("fetching tenant data")
)
And update the CostAttributionLabels(...) method to wrap the tenant provider error:
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrTenantProvider, err)
}
Which you could assert for here like so:
if testcase.expectError {
require.ErrorIs(t, err, TenantProviderError)
}
Following a similar path as limits to pull cost attribution labels for
tenants. Adds a new internal library
cals/cals.gothat provides aninterface that wraps the
TenantProviderclient. Updates all of thewiring from Updated => Scraper to pass along the new cal client.
Starts the process of using the library in
scraper.go, which right nowfinds the cost attribution labels for a specific tenant. Returns an
empty slice if nothing is found.
This will be dependent upon the API changes being released to get cost
attribution labels defined at the tenant level.
My goal is to have a follow up PR that updates the telemeter bits to track executions by cost attribution labels.