[v2] Change e2e jaeger-v2 binary log output to temp file #5431

james-ryans · 2024-05-07T18:20:44Z

Which problem is this PR solving?

Resolves Improve logging in e2e tests #5421

Description of the changes

Instead of throwing binary logs to the parent's output, it now writes the logs to a temp file and only outputs the logs if the test fails. This behavior is similar to the current v1 tests that run docker logs if failed.
Here's the outcome if the test fails.

=== NAME  TestGRPCStorage
    e2e_integration.go:88: Jaeger-v2 logs:
        2024/05/08 00:41:25 application version: git-commit=, git-version=, build-date=
        2024-05-08T00:41:25.557+0700	info	service@v0.99.0/service.go:99	Setting up own telemetry...
        2024-05-08T00:41:25.557+0700	info	service@v0.99.0/telemetry.go:103	Serving metrics	{"address": ":8888", "level": "Normal"}
        2024-05-08T00:41:25.557+0700	info	exporter@v0.99.0/exporter.go:275	Development component. May change in the future.	{"kind": "exporter", "data_type": "traces", "name": "jaeger_storage_exporter"}
        2024-05-08T00:41:25.557+0700	info	service@v0.99.0/service.go:166	Starting jaeger...	{"Version": "git-commit=, git-version=, build-date=", "NumCPU": 8}
        ...
        2024-05-08T00:41:28.151+0700	warn	app/grpc_handler.go:105	trace not found	{"kind": "extension", "name": "jaeger_query", "id": "0000000000000011", "error": "trace not found"}
        2024-05-08T00:41:29.159+0700	warn	app/grpc_handler.go:105	trace not found	{"kind": "extension", "name": "jaeger_query", "id": "0000000000000001", "error": "trace not found"}
        2024-05-08T00:42:20.300+0700	info	exporterhelper/retry_sender.go:118	Exporting failed. Will retry the request after interval.	{"kind": "exporter", "data_type": "traces", "name": "jaeger_storage_exporter", "error": "plugin error: rpc error: code = Unavailable desc = error reading from server: read tcp [::1]:60235->[::1]:17271: read: connection reset by peer", "interval": "4.858861617s"}
--- ❌ FAIL: TestGRPCStorage (71.93s)
    ...
    --- ✅ PASS: TestGRPCStorage/GetLargeSpans (50.57s)
    --- ❌ FAIL: TestGRPCStorage/FindTraces (17.11s)
        --- ✅ PASS: TestGRPCStorage/FindTraces/Tags_in_one_spot_-_Tags (0.02s)
        ...
        --- ❌ FAIL: TestGRPCStorage/FindTraces/Operation_name_+_max_Duration (5.13s)
        ...
        --- ✅ PASS: TestGRPCStorage/FindTraces/Multiple_Traces (0.02s)
❌ FAIL
coverage: 13.0% of statements in ./...
❌ FAIL	github.com/jaegertracing/jaeger/cmd/jaeger/internal/integration	72.620s
❌ FAIL

How was this change tested?

Run locally the STORAGE=grpc SPAN_STORAGE_TYPE=memory make jaeger-v2-storage-integration-test command with success and fail scenario.

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: James Ryans <james.ryans2012@gmail.com>

codecov · 2024-05-07T18:29:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.91%. Comparing base (79d9729) to head (5f6f754).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5431      +/-   ##
==========================================
+ Coverage   94.60%   94.91%   +0.30%     
==========================================
  Files         346      334      -12     
  Lines       16951    16230     -721     
==========================================
- Hits        16037    15404     -633     
+ Misses        712      648      -64     
+ Partials      202      178      -24

Flag	Coverage Δ
badger_v1	`7.97% <ø> (-2.32%)`	⬇️
badger_v2	`1.88% <ø> (-4.48%)`	⬇️
cassandra-3.x-v1	`16.27% <ø> (-1.89%)`	⬇️
cassandra-3.x-v2	`1.81% <ø> (-4.33%)`	⬇️
cassandra-4.x-v1	`16.27% <ø> (-1.89%)`	⬇️
cassandra-4.x-v2	`1.81% <ø> (-4.33%)`	⬇️
elasticsearch-5.x	`1.72% <ø> (-4.02%)`	⬇️
elasticsearch-6.x	`1.73% <ø> (-4.00%)`	⬇️
elasticsearch-7.x	`1.73% <ø> (-4.01%)`	⬇️
elasticsearch-8.x	`1.72% <ø> (-4.01%)`	⬇️
grpc_v1	`10.13% <ø> (-2.46%)`	⬇️
grpc_v2	`7.35% <ø> (?)`
kafka	`9.68% <ø> (-0.31%)`	⬇️
opensearch-1.x	`1.73% <ø> (-4.00%)`	⬇️
opensearch-2.x	`1.72% <ø> (-4.02%)`	⬇️
unittests	`93.11% <ø> (+1.68%)`	⬆️

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.

yurishkuro

a few comments:

since we're writing to files, there's no need to mix stdout and stderr anymore, we can write to two files and dump them separately
I'd say you want to kill the process first, and then read & dump the log files
I am curious if we could create a foldable section in the GH running output. Not sure if it will work when using t.Logf, I assume the special annotation needs to be at the start of the line

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

james-ryans · 2024-05-08T04:49:26Z

This is the output for the group section in my local, since the CI has passed all the tests and I'm not sure if this actually working.

=== NAME  TestGRPCStorage/FindTraces
    logger.go:146: 2024-05-08T11:06:40.783+0700	INFO	Memory storage initialized	{"configuration": {"MaxTraces":0}}
    remote_memory_storage.go:44: Starting in-process remote storage server on :17271
    logger.go:146: 2024-05-08T11:06:40.783+0700	INFO	Starting GRPC server	{"addr": "[::]:17271"}
::group::Jaeger-v2 binary logs
=== NAME  TestGRPCStorage
    e2e_integration.go:101: Jaeger-v2 output logs:
    e2e_integration.go:105: Jaeger-v2 error logs:
        2024/05/08 11:05:41 application version: git-commit=, git-version=, build-date=
        2024-05-08T11:05:41.772+0700	info	service@v0.100.0/service.go:102	Setting up own telemetry...
        2024-05-08T11:05:41.772+0700	info	service@v0.100.0/telemetry.go:103	Serving metrics	{"address": ":8888", "level": "Normal"}
        2024-05-08T11:05:41.772+0700	info	exporter@v0.100.0/exporter.go:275	Development component. May change in the future.	{"kind": "exporter", "data_type": "traces", "name": "jaeger_storage_exporter"}
        2024-05-08T11:05:41.773+0700	info	service@v0.100.0/service.go:169	Starting jaeger...	{"Version": "git-commit=, git-version=, build-date=", "NumCPU": 8}
        ...
::endgroup::
--- ✅ PASS: TestGRPCStorage (59.71s)

cmd/jaeger/internal/integration/e2e_integration.go

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro · 2024-05-08T15:04:34Z

cmd/jaeger/internal/integration/e2e_integration.go

+			fmt.Printf("Jaeger-v2 error logs:\n%s", errLogs)
+			// End of Github Actions foldable section annotation.
+			fmt.Println("::endgroup::")
+		}


perhaps it makes sense to configure codecov to ignore cmd/jaeger/internal/integration/

Signed-off-by: Yuri Shkuro <github@ysh.us>

output jaeger-v2 binary log to temp file

a8c9e41

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro added the changelog:ci Change related to continuous integration / testing label May 8, 2024

yurishkuro reviewed May 8, 2024

View reviewed changes

james-ryans added 3 commits May 8, 2024 10:51

separate stdout and stderr temp files

3c47433

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

output the logs after the process has been killed

3222ac1

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

set binary logs to foldable in GH

766131f

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro approved these changes May 8, 2024

View reviewed changes

cmd/jaeger/internal/integration/e2e_integration.go Show resolved Hide resolved

change binary log to fmt.Printf to keep in sync with group section

dea889c

Signed-off-by: James Ryans <james.ryans2012@gmail.com>

yurishkuro reviewed May 8, 2024

View reviewed changes

yurishkuro approved these changes May 8, 2024

View reviewed changes

yurishkuro marked this pull request as ready for review May 8, 2024 15:14

yurishkuro requested a review from a team as a code owner May 8, 2024 15:14

yurishkuro requested a review from albertteoh May 8, 2024 15:14

add codecov exclusion

5f6f754

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro merged commit 7d21f49 into jaegertracing:main May 8, 2024
40 checks passed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] Change e2e jaeger-v2 binary log output to temp file #5431

[v2] Change e2e jaeger-v2 binary log output to temp file #5431

james-ryans commented May 7, 2024

codecov bot commented May 7, 2024 •

edited

Loading

yurishkuro left a comment

james-ryans commented May 8, 2024

yurishkuro May 8, 2024

[v2] Change e2e jaeger-v2 binary log output to temp file #5431

[v2] Change e2e jaeger-v2 binary log output to temp file #5431

Conversation

james-ryans commented May 7, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

james-ryans commented May 8, 2024

yurishkuro May 8, 2024

Choose a reason for hiding this comment

codecov bot commented May 7, 2024 •

edited

Loading