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

fix: onRun Query offset is made zero #4083

Merged
merged 5 commits into from
Nov 30, 2023
Merged

fix: onRun Query offset is made zero #4083

merged 5 commits into from
Nov 30, 2023

Conversation

palashgdev
Copy link
Contributor

@palashgdev palashgdev commented Nov 27, 2023 •
edited
Loading

@palashgdev palashgdev self-assigned this Nov 27, 2023
@github-actions GitHub Actions
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the bug Something isn't working label Nov 27, 2023
@github-actions GitHub Actions
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@srikanthccv
Copy link
Member

Does the same happen in logs explorer?

@palashgdev
Copy link
Contributor Author

Does the same happen in logs explorer?

yes this was the same behaviour under logs explorer also

@srikanthccv
Copy link
Member

Please fix in both the explorer pages.

@palashgdev
Copy link
Contributor Author

Please fix in both the explorer pages.

@srikanthccv i have fixed in both the pages

srikanthccv
srikanthccv previously approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

This change also resets the page size but that maybe fine.

Rajat-Dabade
Rajat-Dabade previously approved these changes Nov 28, 2023
View reviewed changes
@YounixM
Copy link
Collaborator

YounixM commented Nov 28, 2023

image

the same issue exists in old trace explorer as well. getFilteredSpans doesn't reset offset on run Query

@palashgdev
Copy link
Contributor Author

image

the same issue exists in old trace explorer as well. getFilteredSpans doesn't reset offset on run Query

this pr is only targeting for new trace explorer and logs explorer

srikanthccv
srikanthccv previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

I tested this on the new explorer pages and approving those changes.

Rajat-Dabade
Rajat-Dabade previously approved these changes Nov 28, 2023
View reviewed changes
@palashgdev
Copy link
Contributor Author

@YounixM can you also verify this one ?

makeavish
makeavish reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@makeavish makeavish left a comment

Choose a reason for hiding this comment

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

Page size should not be reset on Run Query, it can be annoying for user

@palashgdev palashgdev changed the title fix: onRun Query Pagination filter is removed fix: onRun Query offset is made zero Nov 29, 2023
@github-actions GitHub Actions
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@srikanthccv
Copy link
Member

@palashgdev This commit added the limit part 5205610. The concern expressed by @makeavish is about the page size. My understanding is that they (limit and pageSize) are different. Does it fix the issue?

@makeavish Please review again.

@srikanthccv
Copy link
Member

I took a quick look and noticed there is no use of PageSize in traces backend and the Limit is used. So I guess the last commit should fix it for traces. @palashgdev Please make sure this doesn't affect logs list view.

@srikanthccv
Copy link
Member

There is no prev/next page since logs is an infinite scroll. I tested the logs explorer. When you go back and apply a new filter the offset is reset.

@srikanthccv
Copy link
Member

@YounixM please review and merge.

@YounixM
Copy link
Collaborator

YounixM commented Nov 30, 2023

Sure @srikanthccv. Have an open question in the PR. will approve and merge post that.

@srikanthccv
Copy link
Member

@YounixM is this the open question you refer to #4083 (comment)? I think the old explorer can be addressed independently.

YounixM
YounixM approved these changes Nov 30, 2023
View reviewed changes
frontend/src/providers/QueryBuilder.tsx Outdated Show resolved Hide resolved
frontend/src/providers/QueryBuilder.tsx Show resolved Hide resolved
@YounixM
Copy link
Collaborator

YounixM commented Nov 30, 2023

It was related to the logic. We can take it up later. Have approved the PR.

@srikanthccv @Rajat-Dabade: Please approve. We can merge it post.

@srikanthccv srikanthccv merged commit 0a77c7a into develop Nov 30, 2023
11 checks passed
@srikanthccv srikanthccv deleted the fix/1110 branch November 30, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YounixM YounixM YounixM approved these changes

@makeavish makeavish makeavish approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

@Rajat-Dabade Rajat-Dabade Awaiting requested review from Rajat-Dabade

Assignees

@palashgdev palashgdev

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@palashgdev @srikanthccv @YounixM @makeavish @Rajat-Dabade