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

fix: use placehold in limit and use proper exists #6667

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Conversation

nityanandagohain
Copy link
Member

@nityanandagohain nityanandagohain commented Dec 18, 2024

Fixes https://github.com/SigNoz/engineering-pod/issues/2098

  • Uses placeholder intead of string formatting for limit queries
  • use proper column name for exists

exist explaination

previously

  • key http.route => http.route != ''
  • key http_url => http_url != ''
    Here since we renamed http.route to attribute_string_http$$route in v3 . There is no column with http.route and the query failed as it was not use the correct name for the key

Now

  • key http.route => attribute_string_http$$route != ''
  • key http_url => http_url != ''
    Now we use context from the static fields to decide

Important

Improve query handling by using placeholders for limit queries and correcting column names for existence checks.

  • Behavior:
    • Use strings.Replace instead of fmt.Sprintf for limit queries in helper.go and v2/helper.go.
    • Correct column name handling for exists and not exists operators in query_builder.go.
  • Functions:
    • Modify existsSubQueryForFixedColumn() in query_builder.go to handle string type columns correctly.
  • Tests:
    • Update TestPrepareTracesQuery in query_builder_test.go to reflect changes in limit query handling.
    • Add test cases for exists and not exists operators in Test_buildTracesFilterQuery in query_builder_test.go.

This description was created by Ellipsis for a6d819e. It will automatically update as commits are pushed.

@github-actions github-actions bot added the bug Something isn't working label Dec 18, 2024
@SigNoz SigNoz deleted a comment from github-actions bot Dec 18, 2024
@SigNoz SigNoz deleted a comment from github-actions bot Dec 18, 2024
Copy link

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

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to a6d819e in 5 minutes and 35 seconds

More details
  • Looked at 131 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/query-service/app/querier/helper.go:193
  • Draft comment:
    Consider defining #LIMIT_PLACEHOLDER as a constant to avoid magic strings and ensure consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from fmt.Sprintf to strings.Replace is appropriate for replacing placeholders in strings. However, the placeholder string should be defined as a constant to avoid magic strings and ensure consistency across the codebase.
2. pkg/query-service/app/querier/v2/helper.go:193
  • Draft comment:
    Consider defining #LIMIT_PLACEHOLDER as a constant to avoid magic strings and ensure consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from fmt.Sprintf to strings.Replace is appropriate for replacing placeholders in strings. However, the placeholder string should be defined as a constant to avoid magic strings and ensure consistency across the codebase.
3. pkg/query-service/app/traces/v4/query_builder.go:328
  • Draft comment:
    Consider defining #LIMIT_PLACEHOLDER as a constant to avoid magic strings and ensure consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from fmt.Sprintf to strings.Replace is appropriate for replacing placeholders in strings. However, the placeholder string should be defined as a constant to avoid magic strings and ensure consistency across the codebase.
4. pkg/query-service/app/querier/helper.go:193
  • Draft comment:
    Avoid using inline styles or direct string replacements in React components. Use external stylesheets, CSS classes, or styled components instead. This issue is also present in pkg/query-service/app/querier/v2/helper.go on line 193.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_en1qzAwj6HAElSbt


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srikanthccv
Copy link
Member

I couldn't follow what change regarding the exists, can you please share more detail?

@nityanandagohain
Copy link
Member Author

@srikanthccv I have updated the description

Copy link

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

2 similar comments
Copy link

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

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

please hold releasing till other messaging changes are merged.

@nityanandagohain nityanandagohain merged commit 85cf4f4 into develop Dec 18, 2024
16 checks passed
@nityanandagohain nityanandagohain deleted the issue_2098 branch December 18, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants