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

[v2][storage] Create v2 query service to operate on otlp data model #6343

Merged
merged 42 commits into from
Dec 31, 2024

Conversation

mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Dec 11, 2024

Which problem is this PR solving?

Description of the changes

  • Implement a v2 version of the query service that operates on the OTLP data model. This PR will be followed up by a series of PRs where this this new query service will be updated with the existing handlers. Once all the handlers have been migrated to use this query service, we can remove the old one.

How was this change tested?

  • Added unit tests

Checklist

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.26%. Comparing base (5901fd8) to head (f163d55).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6343      +/-   ##
==========================================
+ Coverage   96.25%   96.26%   +0.01%     
==========================================
  Files         369      370       +1     
  Lines       21051    21136      +85     
==========================================
+ Hits        20262    20347      +85     
  Misses        604      604              
  Partials      185      185              
Flag Coverage Δ
badger_v1 10.53% <ø> (ø)
badger_v2 2.59% <ø> (ø)
cassandra-4.x-v1-manual 16.41% <ø> (ø)
cassandra-4.x-v2-auto 2.52% <ø> (ø)
cassandra-4.x-v2-manual 2.52% <ø> (ø)
cassandra-5.x-v1-manual 16.41% <ø> (ø)
cassandra-5.x-v2-auto 2.52% <ø> (ø)
cassandra-5.x-v2-manual 2.55% <ø> (ø)
elasticsearch-6.x-v1 20.14% <ø> (ø)
elasticsearch-7.x-v1 20.21% <ø> (ø)
elasticsearch-8.x-v1 20.38% <ø> (+<0.01%) ⬆️
elasticsearch-8.x-v2 2.58% <ø> (ø)
grpc_v1 12.19% <ø> (ø)
grpc_v2 8.97% <ø> (+<0.01%) ⬆️
kafka-3.x-v1 10.37% <ø> (ø)
kafka-3.x-v2 2.59% <ø> (ø)
memory_v2 2.59% <ø> (ø)
opensearch-1.x-v1 20.25% <ø> (ø)
opensearch-2.x-v1 20.26% <ø> (ø)
opensearch-2.x-v2 2.58% <ø> (ø)
tailsampling-processor 0.39% <ø> (ø)
unittests 95.13% <100.00%> (+0.01%) ⬆️

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.

@mahadzaryab1 mahadzaryab1 changed the title [WIP][v2][storage] Create v2 query service to operate on otel data model [WIP][v2][storage] Create v2 query service to operate on otlp data model Dec 12, 2024
mahadzaryab1 added a commit that referenced this pull request Dec 23, 2024
…e on otlp format (#6396)

## Which problem is this PR solving?
- Towards #6344 

## Description of the changes
- Implemented a function `StandardAdjusters` that returns a list of
adjusters to be applied on ptrace.Traces
- This will be used by the v2 query service in
#6343

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

---------

Signed-off-by: Mahad Zaryab <[email protected]>
Comment on lines 147 to 154
func (qs QueryServiceV2) Adjust(tracesIter iter.Seq[[]ptrace.Traces]) {
tracesIter(func(traces []ptrace.Traces) bool {
for _, trace := range traces {
qs.options.Adjuster.Adjust(trace)
}
return true
})
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro Did I understand correctly in that this is what we wanted here? Or did you mean that we should change the underlying adjusters themselves to work on Seq?

Copy link
Member

Choose a reason for hiding this comment

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

yes, you understood correctly. However, because adjusting needs to re-arrange the data I think this func should return Seq[ptrace.Traces] where each item is fully adjusted trace.

Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Dec 24, 2024
@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro Should this be classified as a new feature or a minor feature?

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.

lgtm

cmd/query/app/querysvc/query_service_v2.go Outdated Show resolved Hide resolved
cmd/query/app/querysvc/query_service_v2.go Outdated Show resolved Hide resolved
func (qs QueryServiceV2) Adjust(tracesIter iter.Seq[[]ptrace.Traces]) {
tracesIter(func(traces []ptrace.Traces) bool {
for _, trace := range traces {
qs.options.Adjuster.Adjust(trace)
Copy link
Member

Choose a reason for hiding this comment

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

you cannot assume here that trace is a full trace, you need to clump consecutive chunks if they are for the same trace ID. We can implement a helper function for that that will take Seq[[]ptrace.Traces] and return Seq[ptrace.Traces] where each item is a full trace. Similar to what I was suggesting in https://github.com/jaegertracing/jaeger/pull/6388/files#r1894703201

Comment on lines 147 to 154
func (qs QueryServiceV2) Adjust(tracesIter iter.Seq[[]ptrace.Traces]) {
tracesIter(func(traces []ptrace.Traces) bool {
for _, trace := range traces {
qs.options.Adjuster.Adjust(trace)
}
return true
})
}
Copy link
Member

Choose a reason for hiding this comment

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

yes, you understood correctly. However, because adjusting needs to re-arrange the data I think this func should return Seq[ptrace.Traces] where each item is fully adjusted trace.

Comment on lines 42 to 44
// TODO: Maybe add metrics Storage here
// SupportRegex bool
// SupportTagFilter bool
Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Dec 31, 2024

Choose a reason for hiding this comment

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

@yurishkuro do you have the context for these items? do we still want to do them?

Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 changed the title [WIP][v2][storage] Create v2 query service to operate on otlp data model [v2][storage] Create v2 query service to operate on otlp data model Dec 31, 2024
@mahadzaryab1 mahadzaryab1 marked this pull request as ready for review December 31, 2024 18:38
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner December 31, 2024 18:38
@mahadzaryab1 mahadzaryab1 requested a review from jkowall December 31, 2024 18:38
Signed-off-by: Mahad Zaryab <[email protected]>
// Copyright (c) 2024 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package querysvc
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro is this file path good? currently the adjusters are in querysvc/v2/adjuster/ - should we also move it under querysvc/v2/querysvc/adjuster?

Copy link
Member

Choose a reason for hiding this comment

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

V2/adjuster is good path. But now that you mentioned, I think we could move the whole thing out of query/app into query/internal, as part of the overall cleanup I'm pushing for. But we can do it in a follow up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good - will do in a follow-up PR

@mahadzaryab1 mahadzaryab1 enabled auto-merge (squash) December 31, 2024 19:20
@mahadzaryab1 mahadzaryab1 merged commit ed30d5d into jaegertracing:main Dec 31, 2024
54 checks passed
Manik2708 pushed a commit to Manik2708/jaeger that referenced this pull request Jan 5, 2025
…aegertracing#6343)

## Which problem is this PR solving?
- Towards jaegertracing#6337 

## Description of the changes
- Implement a v2 version of the query service that operates on the OTLP
data model. This PR will be followed up by a series of PRs where this
this new query service will be updated with the existing handlers. Once
all the handlers have been migrated to use this query service, we can
remove the old one.

## How was this change tested?
- Added unit tests

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

---------

Signed-off-by: Mahad Zaryab <[email protected]>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this pull request Jan 5, 2025
…e on otlp format (jaegertracing#6396)

## Which problem is this PR solving?
- Towards jaegertracing#6344 

## Description of the changes
- Implemented a function `StandardAdjusters` that returns a list of
adjusters to be applied on ptrace.Traces
- This will be used by the v2 query service in
jaegertracing#6343

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

---------

Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 deleted the query-service-v2 branch January 6, 2025 00:42
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.

2 participants