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

Add type in products #141

Closed
wants to merge 1 commit into from
Closed

Add type in products #141

wants to merge 1 commit into from

Conversation

zugao
Copy link
Contributor

@zugao zugao commented Jun 1, 2023

Summary

This PR adds column type to the products table. The type column groups a set of targets. For instance products with target 2, 8 and 10 is of type appcat.

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.

@zugao zugao added the enhancement New feature or request label Jun 1, 2023
@zugao zugao requested a review from a team as a code owner June 1, 2023 14:49
Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

You can't retroactively change migrations. You need a new migration to add the column.

Can you link the documentation for this field? Or update the one we have? https://kb.vshn.ch/appuio-cloud/references/architecture/metering-data-flow.html

pkg/invoice/invoice_test.go Outdated Show resolved Hide resolved
@zugao
Copy link
Contributor Author

zugao commented Jun 2, 2023

You can't retroactively change migrations. You need a new migration to add the column.

Ah I see, it's like with percona tools. Understood.

Can you link the documentation for this field? Or update the one we have? https://kb.vshn.ch/appuio-cloud/references/architecture/metering-data-flow.html

Thanks for directions, I was very much unsure what has to be updated.

Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

Can you link the docs?

I have no idea what this field is for without the context you have. cc @corvus-ch

pkg/db/migrations/0004_create_products.sql Outdated Show resolved Hide resolved
@zugao
Copy link
Contributor Author

zugao commented Jun 2, 2023

Can you link the docs?

I will create a PR soon in the docs. Meanwhile you can wait approving this PR.

I have no idea what this field is for without the context you have. cc @corvus-ch

Well we need it for reporting dashboards. The goal is internal to team Schedar. Aldebaran also might want to apply some kind of groupings to its products. The value is not mandatory.

@corvus-ch
Copy link
Member

Well we need it for reporting dashboards. The goal is internal to team Schedar. Aldebaran also might want to apply some kind of groupings to its products. The value is not mandatory.

That is a dangerous path to go. We must be really cautious about what we add to the billing system and why. Note that there is already some, although implicit, grouping available: the source string of products. If segmenting of the product's source string is done right, it could be used to build grouping.

@zugao
Copy link
Contributor Author

zugao commented Jun 2, 2023

Well we need it for reporting dashboards. The goal is internal to team Schedar. Aldebaran also might want to apply some kind of groupings to its products. The value is not mandatory.

That is a dangerous path to go. We must be really cautious about what we add to the billing system and why. Note that there is already some, although implicit, grouping available: the source string of products. If segmenting of the product's source string is done right, it could be used to build grouping.

I personally don't see any issues with this new column, it will greatly reduce the complexity of queries and will eliminate the need to update grafana dashboards. I don't see a good alternative but we can have a short call to dive into this topic as I am not expert with our billing solution.

@zugao
Copy link
Contributor Author

zugao commented Jun 5, 2023

We decided to go with a different approach - changing the source string. Closing this PR

@zugao zugao closed this Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants