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

PR for appcat on change-billing #512

Merged
merged 3 commits into from
Oct 25, 2024
Merged

PR for appcat on change-billing #512

merged 3 commits into from
Oct 25, 2024

Conversation

vshnbot
Copy link
Collaborator

@vshnbot vshnbot commented Oct 21, 2024

Summary

Currently we are getting data for billing from the workloads (deployments and statefulsets) special label appcat.io/billing=true. This approach proved to be ineffective for some services such as VSHNPostgreSQL which is managed by Stackgres. Unfortunately the operator sometimes is managing the pods directly, by passing the workload. In this particular case we end up without the necessary label in the pod definition.

This new approach manages differently the billing architecture. If previously we were having one prometheus rule which would gather unnecessarily difficult specific data, now we have one rule per each instance namespace and it is managed directly by the composition function where we get reliable data from the source.

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.
  • Link this PR to related issues.

Link: vshn/appcat#246

@zugao zugao added the minor label Oct 21, 2024
@zugao zugao force-pushed the appcat/246/change-billing branch from 4df6eb5 to aa24b01 Compare October 21, 2024 13:47
@zugao zugao requested review from a team, Kidswiss, TheBigLee and wejdross and removed request for a team October 21, 2024 13:47
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

LGTM

But please be sure that the metrics on mimir are identical. Ideally roll it out to the lab and run the odoo job then verify with tobru.

@@ -55,7 +55,7 @@ parameters:
appcat:
registry: ghcr.io
repository: vshn/appcat
tag: v4.98.0
tag: change-billing
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't break it with git push --force :D

@zugao zugao force-pushed the appcat/246/change-billing branch from aa24b01 to 9a2b6b9 Compare October 24, 2024 11:22
@zugao zugao requested a review from Kidswiss October 25, 2024 07:24
Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

One more thing and then LGTM

tests/vshn.yml Outdated Show resolved Hide resolved
@@ -157,8 +157,8 @@ local generateCloudAndManaged = function(name)
// For postgresql we have a missmatch between the label and the name in our definition.
local queryName = if name == 'postgres' then name + 'ql' else name;

local managedQuery = 'appcat:metering{label_appuio_io_billing_name="appcat-' + queryName + '",label_appcat_vshn_io_sla="%s", tenant_name!="APPUiO"}';
local cloudQuery = 'appcat:metering{label_appuio_io_billing_name="appcat-' + queryName + '",label_appcat_vshn_io_sla="%s", tenant_name="APPUiO"} * on(label_appuio_io_organization) group_left(sales_order) label_replace(appuio_control_organization_info{namespace="appuio-control-api-production"}, "label_appuio_io_organization", "$1", "organization", "(.*)")';
local managedQuery = 'sum_over_time(appcat:metering{label_appuio_io_billing_name="appcat-' + queryName + '",label_appcat_vshn_io_sla="%s", tenant_name!="APPUiO"}[60:1])/60';
Copy link
Contributor

Choose a reason for hiding this comment

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

I just remembered something:
When we do this exact query via the golang prometheus client, it will be slightly off compared to the GUI and recording rules.

For some weird obscure reason we have to use [59:1] instead of ´[60:1]´.

Refs:
Aldebaran has that in every of their cronjobs: https://github.com/vshn/package-appuio-reporting-aldebaran/blob/master/main.yml#L1015
Note about this the legacy AppCat query: https://github.com/vshn/component-appcat/blob/master/component/promql/appcat.promql#L63

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting

@zugao zugao force-pushed the appcat/246/change-billing branch from 9a2b6b9 to fbfa96c Compare October 25, 2024 07:57
@zugao zugao force-pushed the appcat/246/change-billing branch from fbfa96c to 083d8f5 Compare October 25, 2024 07:59
@zugao zugao requested a review from Kidswiss October 25, 2024 08:08
@zugao zugao merged commit 0aa01fe into master Oct 25, 2024
28 checks passed
@zugao zugao deleted the appcat/246/change-billing branch October 25, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants