[CI][WIP] Scrape metrics at the end of e2e test #6330

chahatsagarmain · 2024-12-09T15:46:06Z

Which problem is this PR solving?

Part of Implement framework to validate backwards compatibility of metrics #6278

Description of the changes

scrape script and usage in cit workflow

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: chahatsagarmain <[email protected]>

codecov · 2024-12-09T15:55:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.11%. Comparing base (737b9be) to head (2244ea9).
Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6330      +/-   ##
==========================================
- Coverage   96.12%   96.11%   -0.02%     
==========================================
  Files         357      360       +3     
  Lines       20537    20479      -58     
==========================================
- Hits        19742    19684      -58     
  Misses        607      607              
  Partials      188      188

Flag	Coverage Δ
badger_v1	`8.96% <ø> (+0.17%)`	⬆️
badger_v2	`1.63% <ø> (+0.01%)`	⬆️
cassandra-4.x-v1-manual	`14.93% <ø> (+0.28%)`	⬆️
cassandra-4.x-v2-auto	`?`
cassandra-4.x-v2-manual	`1.57% <ø> (+0.01%)`	⬆️
cassandra-5.x-v1-manual	`14.93% <ø> (+0.28%)`	⬆️
cassandra-5.x-v2-auto	`1.57% <ø> (+0.01%)`	⬆️
cassandra-5.x-v2-manual	`?`
elasticsearch-6.x-v1	`18.61% <ø> (+0.25%)`	⬆️
elasticsearch-7.x-v1	`18.70% <ø> (+0.26%)`	⬆️
elasticsearch-8.x-v1	`18.85% <ø> (+0.25%)`	⬆️
elasticsearch-8.x-v2	`1.62% <ø> (+<0.01%)`	⬆️
grpc_v1	`10.58% <ø> (+0.34%)`	⬆️
grpc_v2	`7.95% <ø> (+0.14%)`	⬆️
kafka-v1	`9.28% <ø> (+0.81%)`	⬆️
kafka-v2	`1.63% <ø> (+0.01%)`	⬆️
memory_v2	`1.62% <ø> (+<0.01%)`	⬆️
opensearch-1.x-v1	`18.74% <ø> (+0.26%)`	⬆️
opensearch-2.x-v1	`18.74% <ø> (+0.25%)`	⬆️
opensearch-2.x-v2	`1.62% <ø> (+<0.01%)`	⬆️
tailsampling-processor	`0.45% <ø> (+<0.01%)`	⬆️
unittests	`94.99% <ø> (-0.05%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: chahatsagarmain <[email protected]>

scripts/scrape-metrics.sh

.github/workflows/ci-e2e-all.yml

Signed-off-by: chahatsagarmain <[email protected]>

.github/workflows/ci-e2e-badger.yaml

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro · 2024-12-14T00:04:09Z

cmd/jaeger/internal/integration/e2e_integration.go

@@ -152,6 +152,14 @@ func (s *E2EStorageIntegration) e2eInitialize(t *testing.T, storage string) {
 	s.SpanReader, err = createSpanReader(logger, ports.QueryGRPC)
 	require.NoError(t, err)

+	scrapeCmd := exec.Command("./scripts/scrape-metrics.sh", "-t", storage)


a) it's a good idea that we can execute scraping from this test, instead of from individual shell scripts
b) if we do it here there is no need to call a shell script, just execute an HTTP GET request from Go and write output to a file.
c) why is scrape dir "../../../.."? I believe the cwd for test is the root of the repository already
d) I would suggest we create a .gitignore-d directory in the root like .metrics and place all the files there. We don't want to create files that will show up as changes in git.

Yes , we should directly send a get request instead of calling the scrape script

.github/workflows/ci-e2e-all.yml

yurishkuro · 2024-12-14T00:04:57Z

scripts/es-integration-test.sh

@@ -130,6 +130,7 @@ main() {
  build_local_img
  if [[ "${j_version}" == "v2" ]]; then
    STORAGE=${distro} SPAN_STORAGE_TYPE=${distro} make jaeger-v2-storage-integration-test
+


please undo unnecessary changes, they create noise

yurishkuro · 2024-12-14T00:06:24Z

.github/workflows/ci-e2e-cassandra.yml

+      uses: actions/upload-artifact@v3
+      with:
+              name: cassandra-metrics-${{ matrix.version }}
+              path: ./metrics/cassandra_metrics.txt


this job runs in a matrix multiple times. In v1 runs there won't be any metrics. In v2 I don't think we want to upload them with the same file name.

Suggested change

path: ./metrics/cassandra_metrics.txt

path: ./metrics/cassandra_metrics_${{ matrix.version.major }}_${{ matrix.version.schema }}_${{ matrix.jaeger-version }}.txt

@yurishkuro the artifacts will identified with name parameter so we can have simple path for the file ?

please implement downloading them, then we can see

.github/workflows/ci-e2e-cassandra.yml

yurishkuro · 2024-12-14T00:09:26Z

.github/workflows/ci-e2e-cassandra.yml

+      with:
+              name: cassandra-metrics-${{ matrix.version }}
+              path: ./metrics/cassandra_metrics.txt
+              retention-days: 7


this won't be enough for artifacts uploaded for a release (those should be at 60 days)

yurishkuro · 2024-12-14T00:10:11Z

.github/workflows/ci-e2e-all.yml

-
-
-
-


How would we download the artifacts back for comparison with the ones from the release?

https://github.com/actions/download-artifact using this but we need unique name for uploaded artifacts .

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro · 2024-12-14T02:36:45Z

.github/workflows/ci-e2e-cassandra.yml

+      with:
+              name: cassandra-metrics-${{ matrix.version }}
+              path: ./metrics/cassandra_metrics.txt
+              retention-days: 60


60 days could force us to exceed storage quota from GitHub. We only need 60 days for uploads from tagged release, all other uploads can have 7 days retention.

yurishkuro · 2024-12-14T02:39:31Z

cmd/jaeger/internal/integration/e2e_integration.go

@@ -152,6 +152,28 @@ func (s *E2EStorageIntegration) e2eInitialize(t *testing.T, storage string) {
 	s.SpanReader, err = createSpanReader(logger, ports.QueryGRPC)
 	require.NoError(t, err)

+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://localhost:8888/metrics", nil)


a) why would you scrape metrics before the test starts?
b) please move this code to a helper function
c) you can probably schedule this function to run in t.Cleanup() so that it runs just before the binary is stopped

yurishkuro · 2024-12-14T02:39:57Z

scripts/scrape-metrics.sh

@@ -0,0 +1,78 @@
+#!/bin/bash


we don't need this script anymore, right?

Will remove it

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro · 2024-12-15T19:27:53Z

.github/workflows/ci-e2e-badger.yaml

+      with:
+            name: badger-metrics-${{ matrix.version }}
+            path: ./metrics/badger_metrics.txt
+            retention-days: 7


Why do you keep switching between 7 and 60 days? I already explained that we need BOTH of those retention values depending on whether the job runs for a tagged release or not.

how do we know if its a tagged release run or a normal run ?

${{ github.event_name == 'release' && 60 || 7 }} something like this ?

no, we don't run these workflows on release event. When CI runs for a tag I think the github.ref variable has a value like refs/tags/v1.0.0, so we can check for that.

But before we even get into all of that we need to define the main logic for comparing metrics artifacts between the current run and the last release. For example. there is this action https://github.com/benchmark-action/github-action-benchmark that does something similar but for benchmark results. It seems to suggest using actions/cache@v4 to store the results of the tagged release run.

yurishkuro · 2024-12-15T19:28:41Z

.gitignore

@@ -50,3 +50,4 @@ sha256sum.combined.txt
 resource.syso
 .gocache
 test-results.json
+metrics/


Please use dot-name .metrics. I don't need to see this dir in ls.

yurishkuro · 2024-12-15T19:31:02Z

cmd/jaeger/internal/integration/e2e_integration.go

+	err = os.MkdirAll("../../../../metrics", os.ModePerm)
+	if err != nil {
+		t.Fatalf("Failed to create directory: %v", err)
+	}


Suggested change

err = os.MkdirAll("../../../../metrics", os.ModePerm)

if err != nil {

t.Fatalf("Failed to create directory: %v", err)

}

const outputDir = "../../../../.metrics"

require.NoError(t, os.MkdirAll(outputDir, os.ModePerm))

yurishkuro · 2024-12-15T19:32:00Z

cmd/jaeger/internal/integration/e2e_integration.go

+	metricsFile, err := os.Create(fmt.Sprintf("../../../../metrics/%v_metrics.txt", storage))
+	if err != nil {
+		t.Fatalf("Failed to create metrics file: %v", err)
+	}


Suggested change

metricsFile, err := os.Create(fmt.Sprintf("../../../../metrics/%v_metrics.txt", storage))

if err != nil {

t.Fatalf("Failed to create metrics file: %v", err)

}

metricsFile, err := os.Create(fmt.Sprintf("%s/%v_metrics.txt", outputDir, storage))

require.NoError(t, err)

script to scrape metrics

9e51b06

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain requested a review from a team as a code owner December 9, 2024 15:46

chahatsagarmain requested a review from mahadzaryab1 December 9, 2024 15:46

chahatsagarmain added 2 commits December 9, 2024 21:29

minor change

cf2ee87

Signed-off-by: chahatsagarmain <[email protected]>

fix dir path

3472d1e

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain marked this pull request as draft December 9, 2024 16:25

chahatsagarmain changed the title ~~[CI] Scrape metrics at the end of e2e test~~ [CI][WIP] Scrape metrics at the end of e2e test Dec 9, 2024

yurishkuro reviewed Dec 9, 2024

View reviewed changes

scripts/scrape-metrics.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Dec 9, 2024

View reviewed changes

.github/workflows/ci-e2e-all.yml Outdated Show resolved Hide resolved

add metric upload to each test and changes in the scrape script

1c12682

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro reviewed Dec 9, 2024

View reviewed changes

.github/workflows/ci-e2e-badger.yaml Outdated Show resolved Hide resolved

minor changes

d5ae8f5

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain requested a review from yurishkuro December 11, 2024 23:10

chahatsagarmain added 2 commits December 14, 2024 04:25

changes

5170c8e

Signed-off-by: chahatsagarmain <[email protected]>

shell lint

a3d3252

Signed-off-by: chahatsagarmain <[email protected]>

chahatsagarmain marked this pull request as ready for review December 14, 2024 00:00

yurishkuro reviewed Dec 14, 2024

View reviewed changes

minor changes

b0ef4d4

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro reviewed Dec 14, 2024

View reviewed changes

reviewed changes

2244ea9

Signed-off-by: chahatsagarmain <[email protected]>

yurishkuro reviewed Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI][WIP] Scrape metrics at the end of e2e test #6330

[CI][WIP] Scrape metrics at the end of e2e test #6330

chahatsagarmain commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

yurishkuro Dec 14, 2024

chahatsagarmain Dec 14, 2024 •

edited

Loading

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

chahatsagarmain Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

chahatsagarmain Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

chahatsagarmain Dec 14, 2024

yurishkuro Dec 14, 2024

chahatsagarmain Dec 14, 2024

chahatsagarmain Dec 14, 2024

yurishkuro Dec 15, 2024

chahatsagarmain Dec 15, 2024

chahatsagarmain Dec 15, 2024

yurishkuro Dec 15, 2024

yurishkuro Dec 15, 2024

yurishkuro Dec 15, 2024

yurishkuro Dec 15, 2024

	path: ./metrics/cassandra_metrics.txt
	path: ./metrics/cassandra_metrics_${{ matrix.version.major }}_${{ matrix.version.schema }}_${{ matrix.jaeger-version }}.txt

[CI][WIP] Scrape metrics at the end of e2e test #6330

Are you sure you want to change the base?

[CI][WIP] Scrape metrics at the end of e2e test #6330

Conversation

chahatsagarmain commented Dec 9, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

chahatsagarmain Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahatsagarmain commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

chahatsagarmain Dec 14, 2024 •

edited

Loading