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

feat: add logs context #3190

Merged
merged 28 commits into from
Jul 30, 2023
Merged

Conversation

dnazarenkoo
Copy link
Contributor

No description provided.

@request-info
Copy link

request-info bot commented Jul 24, 2023

We would appreciate it if you could provide us with more info about this issue/pr!

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

Duplicate API calls are getting sent, two are sent for previous and two are sent for next. Ideally it should just be one

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

  • While clicking on show 10 more lines before match. It is sending the same id in the payload because of which the same result is returned
  • Same for clicking show 10 more lines after match. The id is not updated.
  • When getting context of logs please send the resource attributes in the filter as described in the implementation details https://www.notion.so/signoz/PRD-View-in-Context-Logs-c9ccb2531730469f8c1f0eeff7bc969b?pvs=4#93e4fc4f059d492e86eb49512bb7e2b7
  • Clicking on edit filter button in logs contexts trigeers unnecessary query range api calls.
  • Adding a filter to the logs conexts modifies the global filter values which shouldn't happen. It should only fetch logs with those filters in the current logs context modal.
  • The button for copy and in contexts appears randomly, have attached screenshot
Screen.Recording.2023-07-25.at.12.11.51.PM.mov

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, a few more is required.

  • Showing 10 lines before match works correctly, but lets say it loaded new lines but the scrolling comes to the top ideally it should be in the lines that was fetched.
  • Showing 10 lines after match. APi call is correct but ordering display is wrong, it should be displayed in reverse order and same issue with scrolling as above.
  • The resource attribute filters that were added from the log line should appear in the filter box

@dnazarenkoo dnazarenkoo marked this pull request as ready for review July 27, 2023 10:21
Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

  • In Showing 10 lines after match, the id to be sent should be the last log id and not the first one.

Copy link
Member

@nityanandagohain nityanandagohain left a comment

Choose a reason for hiding this comment

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

  • Merged array after fetching log lines is wrong, I have attached a video.
  • The button is 10 more log lines for both prev/next logs but in the api call we are sending pageSize as 5
Screen.Recording.2023-07-27.at.5.31.30.PM.mov

@dnazarenkoo
Copy link
Contributor Author

@dnazarenkoo can you please check logs context is not connected or sync with query builder context ?

not connected

@palashgdev
Copy link
Contributor

@dnazarenkoo can you please check logs context is not connected or sync with query builder context ?

not connected

we have to connect the query builder with the logs context

@nityanandagohain
Copy link
Member

@palashgdev what do you mean by logs context connected to query builder context ? Does it mean syncing filters with query builder ?

@palashgdev
Copy link
Contributor

@palashgdev what do you mean by logs context connected to query builder context? Does it mean syncing filters with query builder?

yeah but you shared it will open up with a fresh instance of the where clause...

@palashgdev palashgdev self-requested a review July 30, 2023 11:00
@palashgdev palashgdev merged commit 5f89e84 into SigNoz:develop Jul 30, 2023
9 of 11 checks passed
@ankitnayan ankitnayan mentioned this pull request Aug 2, 2023
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.

None yet

3 participants