feat: update clickhouse reader to support new trace schema

[nityanandagohain]

These PR has all the clickhouse reader changes to support the new v2 table.

The next PR's will be

• integration of the flag
• TTL

FOR #5713

---

Important

Update ClickHouse reader to support new trace schema v3 with schema-specific functions and configurations.

• Schema Support:
  • Add support for trace schema v3 in options.go and reader.go.
  • Introduce constants for v3 tables: defaultTraceIndexTableV3, defaultTraceLocalTableName, defaultTraceResourceTableV3, defaultTraceSummaryTable.
  • Add useTraceNewSchema flag in ClickHouseReader to toggle schemas.
• Functionality:
  • Modify functions like GetServicesList, GetServices, GetServiceOverview, GetTopOperations, GetUsage, SearchTraces, GetTotalSpans, GetSpansInLastHeartBeatInterval, GetTagsInfoInLastHeartBeatInterval to support v3 schema.
  • Add v3-specific functions: SearchTracesV2, GetTraceAggregateAttributesV2, GetTraceAttributeKeysV2, GetTraceAttributeValuesV2, GetSpanAttributeKeysV2.
• Miscellaneous:
  • Update NewOptions and NewReaderFromClickhouseConnection for v3 schema configurations.
  • Add comments for future integration of useTraceNewSchema flag.

^{This description was created by for bc5ab60. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 82bb83a in 2 minutes and 10 seconds

More details

• Looked at 741 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/query-service/app/clickhouseReader/reader.go:1146

• Draft comment:
  Avoid logging raw SQL queries directly as it may expose sensitive information. Consider logging a sanitized version or using placeholders. This is applicable in other parts of the code as well.
• Reason this comment was not posted:
  Marked as duplicate.

2. pkg/query-service/app/clickhouseReader/reader.go:2303

• Draft comment:
  Avoid logging raw SQL queries directly as it may expose sensitive information. Consider logging a sanitized version or using placeholders. This is applicable in other parts of the code as well.
• Reason this comment was not posted:
  Marked as duplicate.

3. pkg/query-service/app/clickhouseReader/reader.go:2435

• Draft comment:
  Avoid logging raw SQL queries directly as it may expose sensitive information. Consider logging a sanitized version or using placeholders. This is applicable in other parts of the code as well.
• Reason this comment was not posted:
  Marked as duplicate.

4. pkg/query-service/app/clickhouseReader/reader.go:3903

• Draft comment:
  Avoid logging raw SQL queries directly as it may expose sensitive information. Consider logging a sanitized version or using placeholders. This is applicable in other parts of the code as well.
• Reason this comment was not posted:
  Marked as duplicate.

5. pkg/query-service/app/clickhouseReader/reader.go:1112

• Draft comment:
  Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. This is also applicable in other parts of the file where telemetry functions are used.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment does not seem to be directly related to any specific change in the diff. It appears to be a general guideline rather than a comment on a specific issue introduced by the changes. Without a direct connection to a change, the comment may not be necessary.
  I might be overlooking a subtle change that indirectly relates to the comment. The comment could be addressing a broader issue that isn't immediately obvious from the diff.
  The comment should be directly related to a change in the diff to be considered relevant. If it addresses a broader issue, it should be more specific about the change it refers to.
  The comment does not seem to be directly related to a specific change in the diff. It appears to be a general guideline rather than addressing a specific issue introduced by the changes. Therefore, it should be removed.

6. pkg/query-service/app/clickhouseReader/reader.go:1095

• Draft comment:
  SearchTracesV2 appears to be a duplicate of SearchTraces with similar core functionality. Consider extending SearchTraces to handle the new schema version instead of duplicating the logic.

• function SearchTraces (reader.go)

• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_FKR8nEILMkA0xLFD

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 28d4b28 in 27 seconds

More details

• Looked at 88 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/query-service/constants/constants.go:722

• Draft comment:
  Consider improving the formatting of the TraceResourceBucketFilterWithServiceName constant for better readability. For example, aligning the SQL keywords and conditions can make it easier to read and maintain.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new constant TraceResourceBucketFilterWithServiceName in constants.go and uses it in reader.go. This is a good practice as it centralizes the query template, making it easier to maintain and update. However, the formatting of the SQL query in the constant could be improved for readability.

2. pkg/query-service/app/clickhouseReader/reader.go:620

• Draft comment:
  Use design tokens or predefined constants for repeated values to maintain consistency and ease of updates. This is correctly done here with constants.TraceResourceBucketFilterWithServiceName.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The code uses a constant for the SQL query filter instead of hardcoding it multiple times, which is a good practice.

Workflow ID: wflow_yzuco46ca0rVXqLJ

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 62d6e07 in 48 seconds

More details

• Looked at 33 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/query-service/app/clickhouseReader/reader.go:721

• Draft comment:
  Ensure that the subtraction of 1800 from queryParams.Start.Unix() is appropriate for all cases. If the interval is less than 30 minutes, this could lead to incorrect results. Consider adding a check or adjustment for smaller intervals.
• Reason this comment was not posted:
  Comment did not seem useful.

2. pkg/query-service/app/clickhouseReader/reader.go:714

• Draft comment:
  The GetTimeSeriesInfo function should not be part of the ClickHouseReader interface as it is not related to ClickHouse. Access this information through the DAO in the telemetry instance instead.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment does not seem to be related to any changes made in the diff. It appears to be addressing existing code, which is not the focus of this review. According to the rules, comments should only be kept if they are about changes made in the diff.
  I might be missing some context about the overall codebase, but based on the diff provided, there is no indication that the GetTimeSeriesInfo function was added or modified in this diff.
  The rules clearly state to focus on changes made in the diff. Since the comment is not about a change, it should be removed.
  Remove the comment as it is not about a change made in the diff.

Workflow ID: wflow_YQJFSKTnEQhHTc77

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 238ab66 in 1 minute and 2 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/app/clickhouseReader/reader.go:4364

• Draft comment:
  Ensure that passing true to isColumn for useLogsNewSchema is the intended behavior for all cases. This change affects how columns are determined in the new schema.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The change in line 4364 modifies the isColumn function call to always pass true for the useLogsNewSchema parameter. This change might be intentional for the new schema, but it should be verified if this is the correct behavior for all cases where isColumn is used. The change should be consistent with the intended logic for the new schema.

2. pkg/query-service/app/clickhouseReader/reader.go:4367

• Draft comment:
  Use r.useTraceNewSchema instead of hardcoding true for the isColumn function call to maintain consistency and clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment suggests using r.useTraceNewSchema for consistency and clarity. However, the useTraceNewSchema variable is currently commented out in the constructor functions, which might indicate it's not being used. The change to hardcode true might be intentional due to the current state of the codebase. Without evidence that useTraceNewSchema is actively used, the comment might not be valid.
  I might be missing the context of why useTraceNewSchema is commented out. It could be planned for future use, or it might have been deprecated. The comment assumes useTraceNewSchema should be used without considering its current state in the code.
  The fact that useTraceNewSchema is commented out in the constructors suggests it might not be intended for use at this time. The hardcoded true could be a temporary or permanent decision based on the current requirements.
  The comment should be deleted because it suggests using a variable that is currently not in use, and the hardcoded true might be intentional given the current state of the code.

Workflow ID: wflow_InPoJuhwyAFEdcpp

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on bc5ab60 in 1 minute and 5 seconds

More details

• Looked at 45 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/query-service/app/clickhouseReader/reader.go:1029

• Draft comment:
  The query uses ts_bucket_start, but this field is not defined in SpanItemV2. Ensure the field is defined in the struct to avoid runtime errors.
• Reason this comment was not posted:
  Comment did not seem useful.

2. pkg/query-service/app/clickhouseReader/reader.go:1026

• Draft comment:
  Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. This applies to functions like GetTimeSeriesInfo, GetSamplesInfoInLastHeartBeatInterval, and GetTotalSamples.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment does not seem to be related to any changes made in the diff. It mentions functions that are not part of the changes, and there is no evidence that the comment addresses a change made in this diff.
  I might be missing the context of the entire codebase, but based on the diff provided, the comment does not seem relevant to the changes made.
  The task is to focus on the diff provided, and the comment does not relate to any changes in the diff. Therefore, it should be removed.
  The comment is not about a change made in the diff and should be removed.

Workflow ID: wflow_AcNNmnTWy28z3Vz5

---

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