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.

Sign up for GitHub

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

Jump to bottom

[Feature]: time parameters for querying trace by id #4150

Closed
alburthoffman opened this issue Jan 11, 2023 · 5 comments · Fixed by #6159
Closed

[Feature]: time parameters for querying trace by id #4150

alburthoffman opened this issue Jan 11, 2023 · 5 comments · Fixed by #6159
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@alburthoffman
Copy link

Requirement

add start_time and end_time as optional parameters in https://github.com/jaegertracing/jaeger-idl/blob/main/proto/api_v2/query.proto#L37

Problem

when using with Granafa console, the trace id view takes a little long time to load.

image

This is because the query has to scan all traces in the db. we have about 4M sampled traces per day and the data is growing, and we store several days data in the database.

Proposal

it would be good to add time parameters when querying by trace id. like tempo API https://grafana.com/docs/tempo/latest/api_docs/#query.

grafana console already have the time window.

Open questions

No response
@yurishkuro
Copy link
Member

What storage backend are you using?

I'm not particularly opposed to this change. Even though I do not recall similar complaints, I assume if someone uses ES as storage with indices rotated regularly, then having a time range as a hint might narrow down which indices to query. However, I am not sure how that would work when people use index alias, it would require some kind of support on ES side to use the time range hint.

Other official Jaeger backends are kv-stores and don't need help looking up by trace ID.

@alburthoffman
Copy link
Author

our backend is clickhouse. Clickhouse is not good at querying item by id, especially when the id is actually a random number.

We simply have too many traces, about 4 billion traces per day. that would need lots of memory if holding them in memory.

for kv store, it can simply ignore the start and end time.

this feature sounds like more general case as the API assume global scanning without any other options.

@vjsamuel
Copy link

vjsamuel commented Feb 1, 2023

@yurishkuro, this would greatly benefit us given the volume of spans we ingest into our click house cluster. we would be more than happy to contribute the enhancement as well.

@yurishkuro
Copy link
Member

go for it.

Speaking of ClickHouse: are you using https://github.com/jaegertracing/jaeger-clickhouse ? I recently opened #4196. There is another implementation in OTel Collector Contrib, which has an additional small table, as I understand for looking up time range based on trace-id

@alburthoffman
Copy link
Author

@yurishkuro Thx for the information. we had evaluated the index table solution long ago. It does not help too much as the trace id is a random number. Clickhouse index does not help with random numbers.

When the traffic volumn is not high, the index table solution can be used.

I will work on the time parameters and send the PR. Thx

@alburthoffman alburthoffman mentioned this issue Mar 2, 2023
Merged
@alburthoffman alburthoffman mentioned this issue Mar 29, 2023
Closed
@alburthoffman alburthoffman mentioned this issue Apr 11, 2023
Merged
@gelicia gelicia mentioned this issue Feb 15, 2024
Open
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Aug 5, 2024
@yurishkuro yurishkuro mentioned this issue Oct 27, 2024
Closed
@rim99 rim99 mentioned this issue Nov 4, 2024
Merged
4 tasks
This was referenced Nov 17, 2024
Merged
Merged
yurishkuro added a commit to jaegertracing/jaeger-idl that referenced this issue Nov 17, 2024
@yurishkuro
Change start/end time to non-nullable (#109)
7728794 
## Which problem is this PR solving?
- Part of jaegertracing/jaeger#4150

## Description of the changes
- Make time values non-nullable to avoid pointers

## How was this change tested?
- In jaegertracing/jaeger#6218

---------

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro added a commit that referenced this issue Nov 18, 2024
@yurishkuro
Change start/end time in GetTrace request to not be pointers (#6218)
684ec99 
## Which problem is this PR solving?
- Part of #4150

## Description of the changes
- Sync to latest jaeger-idl (inculding
jaegertracing/jaeger-idl#109)
- Regenerate protobuf

## How was this change tested?
- CI

---------

Signed-off-by: Yuri Shkuro <[email protected]>
This was referenced Nov 21, 2024
Merged
Merged
yurishkuro added a commit to jaegertracing/jaeger-idl that referenced this issue Nov 21, 2024
@yurishkuro
Remove gogo annotations (#112)
bb13310 
## Which problem is this PR solving?
- Part of jaegertracing/jaeger#4150
- Having gogo annotations in the IDL makes it difficult to users to
build clients

## Description of the changes
- Remove gogo annotations
- Rename num_traces to search_depth

## How was this change tested?
- Tested in jaegertracing/jaeger#6233

---------

Signed-off-by: Yuri Shkuro <[email protected]>
yurishkuro added a commit that referenced this issue Nov 21, 2024
@yurishkuro
Auto-generate gogo annotations for api_v3 (#6233)
c00931f 
## Which problem is this PR solving?
- Part of #4150

## Description of the changes
- Upgrade to new IDL that removes gogo annotation from
api_v3/query.proto
- Add a pre-processor to inject annotations during build
- Fix tests
- Add start/end time to GetTrace test (which was failing in #6228)

## How was this change tested?
- 

## 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: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro mentioned this issue Nov 23, 2024
Merged
4 tasks
This was referenced Nov 24, 2024
Closed
Merged
yurishkuro pushed a commit to jaegertracing/jaeger-idl that referenced this issue Nov 25, 2024
@rim99
[api_v2] Change time fields in ArchiveTraceRequest to non-nullable fo…
de97430 
…r gogo (#113)

## Which problem is this PR solving?
Part of jaegertracing/jaeger#4150

## Description of the changes
- Make sure type of time fields are aligned with other requests

## How was this change tested?
- unit test

## 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: rim99 <[email protected]>
This was referenced Nov 25, 2024
Merged
Merged
yurishkuro pushed a commit that referenced this issue Nov 25, 2024
@rim99
[api_v2] Change time fields in ArchiveTraceRequest to non-nullable (#…
ae0982e 
…6251)

## Which problem is this PR solving?
Part of #4150

## Description of the changes
- related to jaegertracing/jaeger-idl#113

## How was this change tested?
- unit test

Signed-off-by: rim99 <[email protected]>
@rim99 rim99 mentioned this issue Nov 26, 2024
Closed
4 tasks
@yurishkuro yurishkuro mentioned this issue Dec 15, 2024
Merged
yurishkuro added a commit that referenced this issue Dec 15, 2024
@rim99 @yurishkuro
[feat] Add time window for GetTrace in span store interface (#6242)
d6456fb 
## Which problem is this PR solving?
Part of #4150

## Description of the changes

Refactor trace get method in span store interface:
- use struct instead of literal trace id
- add time window in the new struct

## How was this change tested?
- unit test

## 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: rim99 <[email protected]>
Signed-off-by: Zhang Xin <[email protected]>
Signed-off-by: Zhang Xin <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
This was referenced Dec 16, 2024
Merged
Merged
yurishkuro added a commit that referenced this issue Dec 18, 2024
@rim99 @yurishkuro
[api_v3] Add time window for GetTrace http_gateway (#6372)
dd799a0 
## Which problem is this PR solving?
- Part of #4150 

## Description of the changes
- Add optional time window for GetTrace http_gateway

## How was this change tested?
- unit test

## 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: Zhang Xin <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Milestone
No milestone
3 participants
@alburthoffman @yurishkuro @vjsamuel