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 tests to Storage v2 API #6366

Open
6 tasks
yurishkuro opened this issue Dec 15, 2024 · 5 comments · May be fixed by #6388
Open
6 tasks

Upgrade storage integration tests to Storage v2 API #6366

yurishkuro opened this issue Dec 15, 2024 · 5 comments · May be fixed by #6388
Labels
area/storage good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement v2

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Dec 15, 2024

Currently our storage integration tests (plugin/storage/integration/ and cmd/jaeger/internal/integration/) operate in the existing v1 storage APIs. We want to migrate them to use V2 api from storage_v2/.

The way these tests operate is each storage backend has its own entry point to the tests and that entry point is responsible for initializing various storage reader/write APIs

type StorageIntegration struct {
SpanWriter spanstore.Writer
SpanReader spanstore.Reader
ArchiveSpanReader spanstore.Reader
ArchiveSpanWriter spanstore.Writer
DependencyWriter dependencystore.Writer
DependencyReader dependencystore.Reader
SamplingStore samplingstore.Store

We can begin upgrading to v2 API by incrementally swapping these fields to have the corresponding v2 interfaces, e.g. replacing SpanReader spanstore.Reader field with TraceReader tracestore.Reader and adjusting the test functions using that interface accordingly. The backend entry points then can provide TraceReader from the existing v1 SpanReader by wrapping it in storage_v2/v1adapter.

Then in the following PRs we can upgrade the remaining interfaces

  • SpanReader
  • SpanWriter
  • ArchiveSpanReader
  • ArchiveSpanWriter
  • DependencyWriter
  • DependencyReader
@yurishkuro yurishkuro added good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement labels Dec 15, 2024
@yurishkuro
Copy link
Member Author

Sure. You cannot learn if you don't try.

@Ayush-Vish
Copy link

Hi @yurishkuro I hope you're doing well. I'm relatively new to Jaeger and I'm excited about the opportunity to contribute. I’d love to get involved and make some meaningful contributions. I am looking forward to learning from the community!

@yurishkuro
Copy link
Member Author

can you tell how to download all the dependencies for jaeger in order to contribute

There are no dependencies aside from Go tool chain (and we only support Linux-like environment). Please use Discussions for these types of questions, as they are distracting from the main topic of this issue.

their is no integration_test.go

The test is linked in the description.

@ekefan
Copy link

ekefan commented Dec 18, 2024

Hi @yurishkuro,

Currently, the StorageIntegration test methods rely on the model.Trace data structure for traces. Since the storage_v2 API is built around OpenTelemetry's ptrace.Traces (pdata module), I have the following questions:

  1. Do we need to rewrite the StorageIntegration test methods and supporting functions to use ptrace.Traces instead of model.Trace?
  2. For the test fixtures (e.g., JSON files) that are currently formatted for the Jaeger model.Trace structure, do we need to convert or rewrite them to comply with the storage_v2 API's data format? Or is there a recommended way to handle this transition dynamically during tests?

@yurishkuro
Copy link
Member Author

@ekefan yes we need to update the business logic of the tests to use ptrace model. We need to find a way to do that incrementally, not in a single humongous PR. Maybe for some time we will need to live in a state where implementations provide both v1 and v2 storage objects while we gradually transition the tests.

We don't need to change json fixtures just yet, because we can easily transform them from model to ptrace.

@ekefan ekefan linked a pull request Dec 19, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement v2
Projects
None yet
3 participants