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

Verify number of traces in verify-traces steps #509

Merged

Conversation

andreasgerstmayr
Copy link
Collaborator

@andreasgerstmayr andreasgerstmayr commented Jul 12, 2023

  • speed up telemetrygen by generating 10 traces instead of running for 30 seconds
  • use Tempo API where applicable
  • install minio storage before each test which writes or reads from storage (to get deterministic results)

Resolves: #98
Resolves: #490

@andreasgerstmayr
Copy link
Collaborator Author

Unfortunately the diff is a mess because it doesn't detect the renames properly (I had to increment the test step numbers to add a new 00-install-storage.yaml step at the beginning of each smoketest).

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2023

Codecov Report

Merging #509 (052c068) into main (b4fdf61) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   78.66%   78.74%   +0.08%     
==========================================
  Files          60       60              
  Lines        4513     4531      +18     
==========================================
+ Hits         3550     3568      +18     
  Misses        809      809              
  Partials      154      154              
Flag Coverage Δ
unittests 78.74% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
apis/tempo/v1alpha1/tempostack_webhook.go 81.07% <100.00%> (+0.36%) ⬆️

... and 2 files with indirect coverage changes

@andreasgerstmayr andreasgerstmayr force-pushed the update-trace-verification branch 2 times, most recently from 1c99575 to c23bcd1 Compare July 12, 2023 14:33
@andreasgerstmayr andreasgerstmayr marked this pull request as ready for review July 12, 2023 19:10
* speed up telemetrygen by generating 10 traces instead of running
  for 30 seconds
* use Tempo API where applicable

Resolves: grafana#98
Resolves: grafana#490
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
* we need deterministic (empty) storage before tests which write or read
  from the storage

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@andreasgerstmayr andreasgerstmayr marked this pull request as draft July 13, 2023 10:13
…n the upgrade test

* use Jaeger API to keep the upgrade test simple (without mounting TLS
  certs)

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@andreasgerstmayr andreasgerstmayr marked this pull request as ready for review July 13, 2023 11:41
Copy link
Collaborator

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

Maybe, it would make sense to move the "verification of traces" logic to one script and mount it in a volume or something. Or to build our own image to run the tests. Just as a way to make it easier to read the tests files and avoid copying/pasting if we need to modify something.

Also... would it make sense to pin the registry.gitlab.com/gitlab-ci-utils/curl-jq image to an specific version? I suppose there should not be problems and should be something easy to fix just in case.

@andreasgerstmayr
Copy link
Collaborator Author

Maybe, it would make sense to move the "verification of traces" logic to one script and mount it in a volume or something. Or to build our own image to run the tests. Just as a way to make it easier to read the tests files and avoid copying/pasting if we need to modify something.

The scripts are not identical, some query the Tempo API, some the Jaeger API, some use mTLS, some use a different (static) namespace. We could create a parameterized script with all the different use cases, but I'm not sure if it's worth it.

Also... would it make sense to pin the registry.gitlab.com/gitlab-ci-utils/curl-jq image to an specific version? I suppose there should not be problems and should be something easy to fix just in case.

+1, done

Copy link
Collaborator

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

@andreasgerstmayr did you run the tests on OCP?

@andreasgerstmayr
Copy link
Collaborator Author

@andreasgerstmayr did you run the tests on OCP?

Yes

@andreasgerstmayr andreasgerstmayr merged commit 41b128c into grafana:main Jul 26, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

verify-traces kuttl step should check number of traces using Jaeger API Reduce test times for e2e tests
4 participants