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 kafka_span_eval, enabling the feature to avoid 404 in the messaging queue tab #6661

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

shivanshuraj1333
Copy link
Member

@shivanshuraj1333 shivanshuraj1333 commented Dec 18, 2024

Enables the feature by default to avoid 404


Important

Enables KafkaSpanEval by default in constants.go to prevent 404 errors in the messaging queue tab.

  • Behavior:
    • Enables KafkaSpanEval by default in constants.go to prevent 404 errors in the messaging queue tab.

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

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 cb87806 in 11 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/constants/constants.go:93
  • Draft comment:
    Ensure that enabling KAFKA_SPAN_EVAL by default does not have unintended side effects in production environments. Consider documenting this change in the release notes or configuration documentation.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change in the PR is straightforward, changing the default value of an environment variable. This change is consistent with the PR description, which aims to enable a feature by default to avoid 404 errors. The change is safe and does not introduce any logical, performance, or security issues.
2. pkg/query-service/constants/constants.go:93
  • Draft comment:
    No issues found with the change. It adheres to the specified rules.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the PR is straightforward and does not violate any of the specified rules.

Workflow ID: wflow_ybTNxWGzGoXauBdu


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

@shivanshuraj1333
Copy link
Member Author

@prashant-shahi either let's merge this PR or have this flag enabled in all the environments used for demo.

@shivanshuraj1333
Copy link
Member Author

@srikanthccv shall we merge this PR?
I don't see any issues so far with the inner join with ~1 month of data in demo environments, let's see how it plays out.

@srikanthccv
Copy link
Member

No, I am strongly against this idea of joining the trace table with itself enabled everywhere by default. It's going to wreck the whole paid pool users node because it causes high CPU and OOMkill. How much data do you have in demo and what is the resource usage when the query is run in demo.

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.

2 participants