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

enable scenario 4 on staging #6269

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

shivanshuraj1333
Copy link
Member

@shivanshuraj1333 shivanshuraj1333 commented Oct 25, 2024

Important

Enable scenario 4 on staging by setting KAFKA_SPAN_EVAL to true and adjusting Kafka query parameters.

  • Environment:
    • Set KAFKA_SPAN_EVAL to true in .github/workflows/staging-deployment.yaml to enable scenario 4 on staging.
  • Query Parameters:
    • Change query context from partition_latency to producer-topic-throughput in getPartitionOverviewLatencyData() in http_handler.go.
  • Model Changes:
    • Add omitempty to EvalTime in MessagingQueue struct in model.go to omit zero values in JSON serialization.
  • Query Logic:
    • Remove producer-topic-throughput from BuildClickHouseQuery() in translator.go to prevent its use in certain contexts.

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

@shivanshuraj1333 shivanshuraj1333 requested a review from a team as a code owner October 25, 2024 08:58
Copy link

request-info bot commented Oct 25, 2024

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

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 d82626a in 41 seconds

More details
  • Looked at 53 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:3102
  • Draft comment:
    The change from partition_latency to producer-topic-throughput in the BuildQueryRangeParams function call might affect the logic if the query context is not handled properly elsewhere. Ensure that this change aligns with the intended logic and that all related query contexts are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:256
  • Draft comment:
    The removal of producer-topic-throughput from the condition in BuildClickHouseQuery might affect the logic if this query context is used elsewhere. Ensure that this change aligns with the intended logic and that all related query contexts are updated accordingly.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:257
  • Draft comment:
    Avoid using hardcoded strings for queryContext. Consider using constants or enums for better maintainability. This is also applicable in other parts of the function.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_hbaERwQ33xSzmIEu


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

@shivanshuraj1333
Copy link
Member Author

@prashant-shahi have you enabled this?

@prashant-shahi
Copy link
Member

@shivanshuraj1333 Did you get to take a look at this comment? #6269 (comment)

You will need to update docker-compose.testing.yaml to include the envs. That changes can go along with your PR changes.

@shivanshuraj1333 shivanshuraj1333 merged commit 7b18c3b into SigNoz:develop Nov 1, 2024
13 of 15 checks passed
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.

2 participants