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

[Refactor]: Added tag and datatype in options for query builder #3555

Merged
merged 10 commits into from
Dec 20, 2023

Conversation

Rajat-Dabade
Copy link
Contributor

@Rajat-Dabade Rajat-Dabade commented Sep 14, 2023

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.

LGTM, but check with @pranay01 if we can shift the details to the left. I think that will be better.

@Rajat-Dabade
Copy link
Contributor Author

@pranay01 this is the possible solution where to show the type and datatype in the option for the select

Option 1:
Screenshot 2023-09-15 at 2 09 54 PM

Option 2:

Screenshot 2023-09-15 at 2 08 18 PM

Option 3:
@suggested by @YounixM
Screenshot 2023-09-15 at 2 11 36 PM

@palashgdev
Copy link
Contributor

✅ 3

@pranay01
Copy link
Collaborator

@Rajat-Dabade Lets go with Option 1. I had a discussion with Kaushik earlier on this, and from a UX perspective Option 1 made more sense - option 3 confused user with details on data type when his first objective is to select the right variable

@srikanthccv
Copy link
Member

Looking at the code, this is done for the where clause only. There are other filters where we created the tag_/resource_ prefixes. Shouldn't we do the same for aggregate attributes, group by and wherever this was done?

@ankitnayan
Copy link
Collaborator

Looking at the code, this is done for the where clause only. There are other filters where we created the tag_/resource_ prefixes. Shouldn't we do the same for aggregate attributes, group by and wherever this was done?

this is already done in the logs detail view. @nityanandagohain what other places needs to be taken care of?

@nityanandagohain
Copy link
Member

Yeah it's done in log detail view, I think the groupby and aggreagte attribute field is only left.

@nityanandagohain
Copy link
Member

Bumping this, can the changes be aded to groupBY and aggreagte attribute ?

@srikanthccv
Copy link
Member

@Rajat-Dabade please add the same to the group by clause everywhere and aggregate attributes for logs and traces only, because the metric name doesn't have any associated type and the data type is always a number.

@srikanthccv
Copy link
Member

@nityanandagohain PTAL

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.

After selecting, the tag prefix should be removed

image

@Rajat-Dabade
Copy link
Contributor Author

After selecting, the tag prefix should be removed

image

@nityanandagohain Done. Please review it once.

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.

LGTM! But there is another issue where removing field from groupby is slow sometimes, please create a separate for that as it is an existing issue in staging as well.

@srikanthccv
Copy link
Member

@YounixM @palashgdev please review this

@YounixM YounixM removed their request for review December 20, 2023 07:02
@Rajat-Dabade
Copy link
Contributor Author

@YounixM Will take up these changes in subsequent PR.

@Rajat-Dabade Rajat-Dabade merged commit 6b2f03d into SigNoz:develop Dec 20, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logs filter U/X: Improve how attributes are shown in logs filter UI.
8 participants