Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Enable usage of adjusters in api_v2 and api_v3 handlers #6417

Closed
2 tasks done
mahadzaryab1 opened this issue Dec 26, 2024 · 1 comment · Fixed by #6423
Closed
2 tasks done

Enable usage of adjusters in api_v2 and api_v3 handlers #6417

mahadzaryab1 opened this issue Dec 26, 2024 · 1 comment · Fixed by #6423
Assignees
@mahadzaryab1
Labels
enhancement v2

Comments

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Dec 26, 2024 •
edited by yurishkuro
Loading

During the process of building out the v2 query service, we noticed that the adjusters were not being used in the api_v2 and api_v3 handlers. In this issue, we want to enable the usage of the adjusters by default in the v1 query service and expose a flag to turn it off. This will then make the migration to the v2 query service easier.

Tasks
@mahadzaryab1 mahadzaryab1 self-assigned this Dec 26, 2024
@dosubot dosubot bot added the v2 label Dec 26, 2024
This was referenced Dec 26, 2024
Merged
Closed
yurishkuro added a commit to jaegertracing/jaeger-idl that referenced this issue Dec 26, 2024
@mahadzaryab1 @yurishkuro
[query][api_v2/v_3] Add raw_traces parameter to GetTraceRequest and F…
63f6e7e 
…indTracesRequest (#114)

## Which problem is this PR solving?
- Towards jaegertracing/jaeger#6417

## Description of the changes
- Added the `raw_traces` parameter to the `GetTraceRequest` and the
`TraceQueryParameters` in the `FindTracesRequest`

## 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 <mahadzaryab1@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
@mahadzaryab1 mahadzaryab1 mentioned this issue Dec 26, 2024
Merged
4 tasks
yurishkuro added a commit that referenced this issue Dec 26, 2024
@mahadzaryab1 @yurishkuro
[chore] Update idl submodule (#6418)
2451730 
## Which problem is this PR solving?
- Towards #6417

## Description of the changes
- Updates the `idl` submodule to the latest commit on main which pulls
in the new `raw_traces` flags added to the api_v2 and api_v3 requests

## 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 <mahadzaryab1@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
This was referenced Dec 27, 2024
Merged
Merged
yurishkuro pushed a commit that referenced this issue Dec 27, 2024
@mahadzaryab1
[v1][adjuster] Change v1 adjuster interface to not return error and m…
e6caacb 
…odify trace in place (#6426)

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

## Description of the changes
- The v1 adjuster interface returns an error even though none of the
adjusters ever return an error. This was leading to handling errors that
would never be thrown. This PR simplifies the interface by removing the
error return.
- The v1 adjuster interface was also returning the trace that it
modified. However, all the adjusters currently just take the trace in as
a pointer and modify it in place so this return was not necessary.

## How was this change tested?
- CI

## 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 <mahadzaryab1@gmail.com>
@mahadzaryab1 mahadzaryab1 mentioned this issue Dec 28, 2024
Merged
4 tasks
yurishkuro pushed a commit that referenced this issue Dec 28, 2024
@mahadzaryab1
[refactor][query] Propagate RawTraces flag to query service (#6438)
a6616fb 
## Which problem is this PR solving?
- Towards #6417

## Description of the changes
- This PR defines `GetTraceParamaters` and `TraceQueryParameters` inside
`package querysvc` that are currently just wrappers around their
`package spanstore` counterparts.
- This is done so that additional parameters can be passed into the
query service, like the `RawTraces` flag, without having to extend the
parameters that are passed into the storage implementations.

## How was this change tested?
- CI

## 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 <mahadzaryab1@gmail.com>
@yurishkuro
Copy link
Member

let's try to manage linkage between issues. I just add this is as a child of #6337.

Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
@mahadzaryab1 @Manik2708
[query] Enable trace adjusters in api_v2 and api_v3 handlers (jaegert…
4be4a35 
…racing#6423)

## Which problem is this PR solving?
- Resolves jaegertracing#6417

## Description of the changes
- 🛑 This PR contains a breaking change for the api_v2 handler.
- All
[GetTrace](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v2/query.proto#L37)
and
[FindTraces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v2/query.proto#L125)
requests will apply
[these](https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/querysvc/adjusters.go#L17-L24)
adjusters/enrichments on the trace by default.
- In order to disable the adjusters for the `FindTraces` request, set
the
[raw_traces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v2/query.proto#L122)
flag in the query parameters to `true`.
- In order to disable the adjusters for the `GetTrace` request, set the
[raw_traces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v2/query.proto#L56)
flag in the request to `true`.
- 🛑 This PR contains a breaking change for the api_v3 handler.
- All
[GetTrace](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L27)
and
[FindTraces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L88)
requests will apply
[these](https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/querysvc/adjusters.go#L17-L24)
adjusters/enrichments on the trace by default.
- In order to disable the adjusters for the `FindTraces` request, set
the
[raw_traces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L84)
flag in the query parameters to `true`.
- In order to disable the adjusters for the `GetTrace` request, set the
[raw_traces](https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v3/query_service.proto#L40)
flag in the request to `true`.

## 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`: `npm run lint` and `npm run test`

---------

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <43658574+mahadzaryab1@users.noreply.github.com>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
@mahadzaryab1 @yurishkuro @Manik2708
[chore] Update idl submodule (jaegertracing#6418)
29b4c9f 
## Which problem is this PR solving?
- Towards jaegertracing#6417

## Description of the changes
- Updates the `idl` submodule to the latest commit on main which pulls
in the new `raw_traces` flags added to the api_v2 and api_v3 requests

## 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 <mahadzaryab1@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Manik2708 pushed a commit to Manik2708/jaeger that referenced this issue Jan 5, 2025
@mahadzaryab1 @Manik2708
[v1][adjuster] Change v1 adjuster interface to not return error and m…
1df828e 
…odify trace in place (jaegertracing#6426)

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

## Description of the changes
- The v1 adjuster interface returns an error even though none of the
adjusters ever return an error. This was leading to handling errors that
would never be thrown. This PR simplifies the interface by removing the
error return.
- The v1 adjuster interface was also returning the trace that it
modified. However, all the adjusters currently just take the trace in as
a pointer and modify it in place so this return was not necessary.

## How was this change tested?
- CI

## 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 <mahadzaryab1@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees

@mahadzaryab1 mahadzaryab1

Labels
enhancement v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[query] Enable trace adjusters in api_v2 and api_v3 handlers mahadzaryab1/jaeger
2 participants
@yurishkuro @mahadzaryab1