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

export: Added "promtest" framework allowing tests against GCM and Prometheus; Added counter test. #634

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

bwplotka
Copy link
Collaborator

@bwplotka bwplotka commented Oct 17, 2023

This PR:

  • Adds pkg/export/gcm/promtest pkg that contains "IngestionTest" framework.
  • (The only change to export pkg) pkg/export: Adding a config option that allows passing credentials content instead of files.
  • Adds pkg/export/gcm/export_gcm_test.go that implements a simple counter test that showcases the framework use.

This uses docker the e2e framework I maintain with some Thanos maintainers for Prometheus test and local export code for "GMP" test. The "local export" is so we can use local debugger and test EVERY export change in this repo.

Tests with Prometheus enabled, takes N samples * 5 seconds. We could take scrape interval to 1s, but it might kill our CI CPU. We can also disable Prometheus tests for CI and run it only locally.

Those tests will also NOT run for contributors as it requires secret.

In future PRs we can:

  • Move to Cloud build for stronger security guarantees for our test SA.
  • Add test for new logic for resets.
  • Add GMP + GCM test (docker container too)
  • Add tests for other types, different queries (e.g. rates) etc - sky is the limit.

@bwplotka
Copy link
Collaborator Author

bwplotka commented Oct 17, 2023

Tests passed without mention about skipping anything, but I see it ran within 3s which is not realistic so assuming it was skipped - investigating.

Added Presubmit / test-export-gcm (pull_request) that runs just that test.

@bwplotka bwplotka force-pushed the bwplotka/reset2 branch 6 times, most recently from a4ec6e9 to abb06d9 Compare October 17, 2023 14:03
…metheus; Added counter test.

Signed-off-by: bwplotka <bwplotka@google.com>
@bwplotka
Copy link
Collaborator Author

All should work, PTAL @TheSpiritXIII @pintohutch

Copy link
Member

@TheSpiritXIII TheSpiritXIII left a comment

Choose a reason for hiding this comment

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

Great work on this large PR! Left some initial feedback. :)

.gitignore Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
pkg/export/gcm/promtest/promtest.go Show resolved Hide resolved
pkg/export/gcm/promtest/promtest.go Outdated Show resolved Hide resolved
pkg/export/gcm/promtest/prometheus.go Outdated Show resolved Hide resolved
pkg/export/gcm/promtest/prometheus.go Show resolved Hide resolved
pkg/export/gcm/promtest/prometheus.go Outdated Show resolved Hide resolved
pkg/export/gcm/export_gcm_test.go Show resolved Hide resolved
pkg/export/gcm/export_gcm_test.go Show resolved Hide resolved
@bwplotka
Copy link
Collaborator Author

bwplotka commented Oct 19, 2023

Thanks for review! Address all comments so far @TheSpiritXIII (:

pkg/export/export.go Outdated Show resolved Hide resolved
@TheSpiritXIII
Copy link
Member

@bwplotka I'm going to approve. Only nit is the Println. Please address and then you can merge. 🙂👍

Signed-off-by: bwplotka <bwplotka@google.com>
@pintohutch
Copy link
Collaborator

This is great! We'd love a demo walkthrough of the e2e package at some point 😄

In general, this PR was a little intimidating in size. Can we try to put up smaller diffs for review?

WDYT about daisy-chaining baby PRs so we don't hinder development while lowering the review burden?

@bwplotka bwplotka merged commit 1d8832a into main Oct 23, 2023
7 checks passed
@bwplotka
Copy link
Collaborator Author

Thanks @TheSpiritXIII for review and @pintohutch for feedback.

This could be split a bit yea (e.g I could split local export AND e2e Prometheus into two), let's try that next time! The beauty of one atomic PR is that reviewer understands clearly why this complexity and e2e dependency is needed. There is also question PR author time, given chaining PRs in on GitHub is non trivial - definitely it would be good idea to document how we want to do it as a team 🤗

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