Skip to content
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

SWATCH-3295: Normalize the payg metrics to use the same tags and format #4159

Merged
merged 7 commits into from
Feb 11, 2025

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Feb 5, 2025

Jira issue: SWATCH-3295

Depends on #4157

Description

All the payg metrics should have the same tags, units and use the same format to be used in the grafana dashboard.

This pull request addresses the following changes:

  • Change format of metric ID to use the metric ID code instead of the uppercase format
    Some metrics were using one format or the another. All the metrics should use the same one.

  • Wrong billing_provider_id in "swatch_tally_tallied_usage_total", it should be "billing_provider"

  • The metric "swatch_tally_tallied_usage_total" was double counting. Fixed by reusing the logic to exclude duplicate snaps.

  • The metrics "swatch_contract_usage_total", "swatch_billable_usage_total" and "swatch_producer_metered_total" were expressed in billing units instead of metric units. We need to use the metric units, so the numbers in the grafana dashboard are consistent.

  • Reverts commit d831fdc. We're not using the json format anylonger in tests.

Testing

IQE Test MR: https://gitlab.cee.redhat.com/insights-qe/iqe-rhsm-subscriptions-plugin/-/merge_requests/1040

@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-3295 branch from 4ca3d92 to baee756 Compare February 5, 2025 09:12
@Sgitario Sgitario marked this pull request as draft February 5, 2025 09:58
@Sgitario Sgitario changed the title SWATCH-3295: Wrong format for metric_id in ingested usage total counter SWATCH-3295: Normalize the payg metrics to use the same tags and format Feb 5, 2025
@kahowell
Copy link
Contributor

kahowell commented Feb 5, 2025

I prefer the formatting we use in our config files: Instance-hours over INSTANCE_HOURS. Is it feasible to normalize to that format instead of the upper case version?

@Sgitario
Copy link
Contributor Author

Sgitario commented Feb 6, 2025

I prefer the formatting we use in our config files: Instance-hours over INSTANCE_HOURS. Is it feasible to normalize to that format instead of the upper case version?

I also prefer the same formatting we use in our config files, will change it for all the payg metrics.

@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-3295 branch 2 times, most recently from 371d2c8 to 6c44867 Compare February 6, 2025 09:18
The counter "swatch_metrics_ingested_usage_total" is being displaying the metric as:
```
swatch_metrics_ingested_usage_total{billing_provider="aws",metric_id="Instance-hours",product="rosa"} 609.8702973568641
```

And it should be using the upper format:

```
swatch_metrics_ingested_usage_total{billing_provider="aws",metric_id="INSTANCE_HOURS",product="rosa"} 609.8702973568641
```

This is to work as the other metrics/counters.
@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-3295 branch from 6cf175e to 86c863e Compare February 7, 2025 06:49
@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-3295 branch from 86c863e to 2f400ac Compare February 7, 2025 07:26
@Sgitario Sgitario marked this pull request as ready for review February 7, 2025 07:29
@wottop wottop self-assigned this Feb 7, 2025
"product",
"rosa",
"metric_id",
MetricIdUtils.getInstanceHours().getValue(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are some of these toUpperCaseFormatted while others are getValue? Shouldn't it be the same call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what toUpperCaseFormatted do you exactly mean, but in general:

  • the only logic that does change is about processing the payg metrics, where we'll use the metric format as shown in our configuration files (not the upper case format)
  • the rest of the existing logic does not change, it keeps using the format that was using before (regardless if it's the upper case formatted or the value)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I thought we were standardizing the metric values across all usages.

@Sgitario
Copy link
Contributor Author

/retest

2 similar comments
@Sgitario
Copy link
Contributor Author

/retest

@Sgitario
Copy link
Contributor Author

/retest

@Sgitario Sgitario requested a review from wottop February 10, 2025 11:30
@kartikshahc kartikshahc self-requested a review February 10, 2025 13:22
@Sgitario
Copy link
Contributor Author

/retest

1 similar comment
@Sgitario
Copy link
Contributor Author

/retest

@InsightsDroid
Copy link
Collaborator

IQE Tests Summary Report

@Sgitario Sgitario merged commit d300d49 into main Feb 11, 2025
18 of 19 checks passed
@Sgitario Sgitario deleted the jcarvaja/SWATCH-3295 branch February 11, 2025 05:55
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.

4 participants