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

csv export: override default max days (default: 90d) #2165

Merged
merged 1 commit into from Oct 10, 2023

Conversation

a7i
Copy link
Contributor

@a7i a7i commented Sep 15, 2023

What does this PR change?

  • OpenCost docs mention that CSV Export includes previous day data (ref). The query here, however, requests 90d of data which is not allowed by AWS Managed Prometheus (by default). This allows the user to set the duration desired

Does this PR relate to any other PRs?

  • No

How will this PR impact users?

  • for existing setup, the duration stays as 90d
  • if env is set, it will use the duration as provided

Does this PR address any GitHub or Zendesk issues?

How was this PR tested?

  • Unit test and dev environment

Does this PR require changes to documentation?

  • No

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

  • yes

@a7i a7i force-pushed the amir/allocation-date-range-env branch 3 times, most recently from 9089c8e to a1490f6 Compare September 15, 2023 02:13
@mattray mattray added opencost OpenCost issues vs. external/downstream P2 Estimated Priority (P0 is highest, P4 is lowest) E2 Estimated level of Effort (1 is easiest, 4 is hardest) labels Sep 15, 2023
@mattray
Copy link
Collaborator

mattray commented Sep 15, 2023

Thanks for the PR (with tests)! I'm going to ping @r2k1 and his team since they wrote the original feature, hopefully he'll weigh in.

Copy link
Contributor

@r2k1 r2k1 left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. I would only ask to rename the ENV variable.

The 90 days selection was arbitrary, chosen for checking the oldest data in Prometheus. If this causes issues on some Prometheus setups, we can reduce it. I don't anticipate problems with existing deployments. However, reducing this period would affect the data amount exported on initial runs where opencost has been active for extended periods without CSV export.

An alternative is to execute multiple 30-day range queries. This may eliminate the need from users to configure an extra ENV variable.

pkg/env/costmodelenv.go Outdated Show resolved Hide resolved
pkg/env/costmodelenv.go Outdated Show resolved Hide resolved
@a7i a7i force-pushed the amir/allocation-date-range-env branch from a1490f6 to 1372b08 Compare September 19, 2023 18:17
@a7i a7i requested a review from r2k1 September 19, 2023 18:17
@a7i a7i force-pushed the amir/allocation-date-range-env branch from 1372b08 to 1e41880 Compare September 19, 2023 18:19
@a7i a7i changed the title csv export: override allocation date range via env csv export: override default max days (default: 90d) Sep 20, 2023
@mattray mattray added the P1 Estimated Priority (P0 is highest, P4 is lowest) label Sep 20, 2023
@mattray
Copy link
Collaborator

mattray commented Sep 20, 2023

With @r2k1's approval I'm going to get this lined up for inclusion in 1.107. Thanks!

@mattray mattray removed the P2 Estimated Priority (P0 is highest, P4 is lowest) label Sep 21, 2023
@nikovacevic nikovacevic force-pushed the amir/allocation-date-range-env branch from f3f038f to 65be6ae Compare October 9, 2023 21:58
Signed-off-by: Amir Alavi <amiralavi7@gmail.com>
@nikovacevic nikovacevic merged commit a275aac into opencost:develop Oct 10, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2 Estimated level of Effort (1 is easiest, 4 is hardest) opencost OpenCost issues vs. external/downstream P1 Estimated Priority (P0 is highest, P4 is lowest)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

csv export cost model: data query interval should be 1 day instead of 90 days
6 participants