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

Upgrade storage integration test: Use V2 Archive ReaderWriter #6489

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ekefan
Copy link
Contributor

@ekefan ekefan commented Jan 6, 2025

Which problem is this PR solving?

Description of the changes

  • Incrementally swaps the fields of StorageIntegration to align with v2 storage api

    • replaced ArchiveSpanReader with ArchiveTraceReader
    • replaced ArchiveSpanWriter with ArchiveTraceWriter
  • Updates test functions accordingly to work with the updated fields

How was this change tested?

  • CI tests

Checklist

@ekefan ekefan requested a review from a team as a code owner January 6, 2025 00:51
@ekefan ekefan requested a review from pavolloffay January 6, 2025 00:51
@ekefan ekefan force-pushed the archive-reader-writer branch from 2ca2f23 to 9ad6870 Compare January 6, 2025 00:53
@ekefan ekefan marked this pull request as draft January 6, 2025 02:26
@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Jan 6, 2025
@yurishkuro yurishkuro marked this pull request as ready for review January 6, 2025 02:51
@yurishkuro yurishkuro enabled auto-merge (squash) January 6, 2025 02:52
@yurishkuro yurishkuro disabled auto-merge January 6, 2025 02:54
@yurishkuro
Copy link
Member

looks like some archive tests are failing

    --- ❌ FAIL: TestElasticsearchStorage/ArchiveTrace (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x22fe57b]

goroutine 65 [running]:
testing.tRunner.func1.2({0x2532840, 0x3a10eb0})
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1632 +0x3fc
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1635 +0x6b6
panic({0x2532840?, 0x3a10eb0?})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/panic.go:785 +0x132
github.com/jaegertracing/jaeger/plugin/storage/integration.(*StorageIntegration).testArchiveTrace.func1(0x6c0f40?)
	/home/runner/work/jaeger/jaeger/plugin/storage/integration/integration.go:214 +0x47b
github.com/jaegertracing/jaeger/plugin/storage/integration.(*StorageIntegration).waitForCondition(0x2a7de18?, 0xc000523040, 0xc000439e58)
	/home/runner/work/jaeger/jaeger/plugin/storage/integration/integration.go:128 +0x165

@ekefan
Copy link
Contributor Author

ekefan commented Jan 6, 2025

there's no new change with the recent push... just trying to update the branch.

I will get back to these issues later in my day.

@ekefan ekefan force-pushed the archive-reader-writer branch from aaab620 to a43bc9a Compare January 7, 2025 09:16
@ekefan
Copy link
Contributor Author

ekefan commented Jan 7, 2025

Hello @yurishkuro,

I can't find the cause of this bug, but for elastic search and open search, the testArchiveTrace test fails; the archive trace reader returns an empty trace...
I want to try updating the reader before the writer, but I'd be glad to get any help or way forward from you.
Thanks alot.

@yurishkuro
Copy link
Member

Did you look at the logs? I see nil pointer segfaults, that should be easy to troubleshoot.

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.00%. Comparing base (20bdd8d) to head (5d23f27).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6489   +/-   ##
=======================================
  Coverage   96.00%   96.00%           
=======================================
  Files         365      365           
  Lines       20616    20616           
=======================================
  Hits        19792    19792           
  Misses        626      626           
  Partials      198      198           
Flag Coverage Δ
badger_v1 9.92% <ø> (ø)
badger_v2 1.84% <ø> (ø)
cassandra-4.x-v1-manual 15.08% <ø> (ø)
cassandra-4.x-v2-auto 1.83% <ø> (ø)
cassandra-4.x-v2-manual 1.83% <ø> (ø)
cassandra-5.x-v1-manual 15.08% <ø> (ø)
cassandra-5.x-v2-auto 1.83% <ø> (ø)
cassandra-5.x-v2-manual 1.83% <ø> (ø)
elasticsearch-6.x-v1 19.30% <ø> (ø)
elasticsearch-7.x-v1 19.38% <ø> (ø)
elasticsearch-8.x-v1 19.56% <ø> (ø)
elasticsearch-8.x-v2 1.95% <ø> (ø)
grpc_v1 11.04% <ø> (ø)
grpc_v2 7.89% <ø> (ø)
kafka-3.x-v1 10.21% <ø> (ø)
kafka-3.x-v2 1.84% <ø> (ø)
memory_v2 1.84% <ø> (ø)
opensearch-1.x-v1 19.44% <ø> (ø)
opensearch-2.x-v1 19.44% <ø> (ø)
opensearch-2.x-v2 1.84% <ø> (ø)
tailsampling-processor 0.48% <ø> (ø)
unittests 94.81% <ø> (ø)

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.

@ekefan ekefan force-pushed the archive-reader-writer branch from ccf0435 to 54a0a07 Compare January 8, 2025 08:46
@ekefan ekefan marked this pull request as draft January 11, 2025 05:56
@ekefan ekefan changed the title Upgrade storage integration test: Use V2 Archive ReaderWriter [WIP] Upgrade storage integration test: Use V2 Archive ReaderWriter Jan 11, 2025
@ekefan ekefan closed this Jan 14, 2025
@ekefan ekefan force-pushed the archive-reader-writer branch from 1febf80 to c678a64 Compare January 14, 2025 04:44
@ekefan ekefan reopened this Jan 14, 2025
@ekefan ekefan force-pushed the archive-reader-writer branch from 7e58074 to cc0a508 Compare January 17, 2025 08:07
@ekefan ekefan force-pushed the archive-reader-writer branch from cc0a508 to 7486084 Compare January 17, 2025 19:01
@ekefan ekefan marked this pull request as ready for review January 17, 2025 19:54
@ekefan ekefan force-pushed the archive-reader-writer branch 2 times, most recently from fb698f7 to 2d8a157 Compare January 17, 2025 22:24
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

plugin/storage/integration/integration.go Outdated Show resolved Hide resolved
storage_v2/v1adapter/tracereader.go Outdated Show resolved Hide resolved
@ekefan ekefan force-pushed the archive-reader-writer branch from 2d8a157 to 120f2f1 Compare January 18, 2025 08:01
@ekefan ekefan marked this pull request as draft January 18, 2025 08:03
@ekefan
Copy link
Contributor Author

ekefan commented Jan 18, 2025

@yurishkuro, @mahadzaryab1, I have two questions please:

  • When all jaeger storage backends are upgraded to v2, will the Archive ReaderWriter fields still exist in StorageIntegration for the storage integration tests?
  • Will testArchiveTrace still be part of the integration tests?

@mahadzaryab1
Copy link
Collaborator

I'm currently in the process of trying to completely phase out the difference between primary and archive interfaces in #6567. If that can be pulled off, then this PR can initialize primary and archive storage in the same way.

@ekefan
Copy link
Contributor Author

ekefan commented Jan 21, 2025

@yurishkuro, @mahadzaryab1, I have two questions please:

  • When all jaeger storage backends are upgraded to v2, will the Archive ReaderWriter fields still exist in StorageIntegration for the storage integration tests?
  • Will testArchiveTrace still be part of the integration tests?

My question has been answered in the description of #6567

@ekefan ekefan force-pushed the archive-reader-writer branch 3 times, most recently from a2b32ba to 71b5e38 Compare January 26, 2025 23:20
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
Signed-off-by: Emmanuel Emonueje Ebenezer <eebenezer949@gmail.com>
@ekefan ekefan force-pushed the archive-reader-writer branch from 4df2c11 to 5d23f27 Compare January 28, 2025 02:34
@ekefan ekefan marked this pull request as ready for review January 28, 2025 02:45
@ekefan ekefan changed the title [WIP] Upgrade storage integration test: Use V2 Archive ReaderWriter Upgrade storage integration test: Use V2 Archive ReaderWriter Jan 28, 2025
@mahadzaryab1
Copy link
Collaborator

@ekefan My apologies that this PR has been held back. I think it might be good to hold off on this PR until #6625 is merged. That should greatly simplify this PR and will likely only require the archive reader/writer to be upgraded in one place.

@ekefan
Copy link
Contributor Author

ekefan commented Jan 28, 2025

@ekefan My apologies that this PR has been held back. I think it might be good to hold off on this PR until #6625 is merged. That should greatly simplify this PR and will likely only require the archive reader/writer to be upgraded in one place.

@mahadzaryab1 The problem was never with the way es or os was reading (as I was suspecting)... And since #6567 was merged, I discovered that it wasn't the distinction between archive or primary data either, I was not waiting for the storage backend to update documents before accessing the traces 😂😂. But its done now😃.

#6625 makes total sense, we can just implement Archive Trace Reader-Writer as I have done here, then close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants