feat(summary): added update metrics metadata api

[aniketio-ctrl]

Summary

Added update metrics metadata api, which is used to update metrics metadata , we are storing this info in a new table and we are going to fetch this details from cache whenever we need details regarding updated metrics, we are also pre loading cache after the start of our clickhouse reader

---

Important

Added an API to update metrics metadata, including server initialization, data handling, model changes, and query execution updates.

• API Addition:
  • Added UpdateMetricsMetadata API in http_handler.go to update metrics metadata.
  • New route /api/v1/metrics/{metric_name}/metadata added in MetricExplorerRoutes().
• Server Initialization:
  • Preload metrics metadata in NewServer() in server.go using PreloadMetricsMetadata().
• Data Handling:
  • Added UpdateMetricsMetadata and GetUpdatedMetricsMetadata functions in reader.go.
  • Added DeleteMetricsMetadata function in reader.go.
  • Added ParseUpdateMetricsMetadataParams() in parser.go for request parsing.
• Model Changes:
  • Added UpdateMetricsMetadata struct in updatedMetricsMetadata.go.
  • Added UpdateMetricsMetadataRequest struct in summary.go.
• Query Execution:
  • Modified runBuilderQuery() in helper.go and v2/helper.go to use updated metrics metadata.
• Constants:
  • Added UpdatedMetricsMetadataCachePrefix in constants.go.

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

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 487fe7f in 3 minutes and 26 seconds

More details

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

1. pkg/query-service/model/updatedMetricsMetadata.go:1

• Draft comment:
  The implementation of UpdateMetricsMetadata looks correct. The struct’s json and ch tags are properly defined, and the MarshalBinary/UnmarshalBinary methods correctly use json.Marshal/Unmarshal. Ensure corresponding unit tests exist to validate serialization/deserialization.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. pkg/query-service/app/http_handler.go:152

• Draft comment:
  The error log messages in UpdateMetricsMetadataData are copy‐pasted from other functions. Consider updating them to reference 'update metrics metadata' rather than 'metric query range' or 'inspect metrics data'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/metricsexplorer/parser.go:92

• Draft comment:
  ParseUpdateMetricsMetadataParams looks OK, but consider adding validations for required fields if needed.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While the suggestion seems reasonable since other similar functions do validation, we don't have access to UpdateMetricsMetadataRequest struct definition to know what fields are required. Without seeing the struct definition, we can't be certain that any validation is actually needed. The comment is speculative with "if needed" and doesn't provide specific guidance.
  The comment could be identifying a real issue since validation is a common pattern in this file. We might be missing important context about the UpdateMetricsMetadataRequest structure.
  However, without seeing the struct definition, we can't be certain that validation is needed. The comment is too speculative with "if needed" and doesn't provide actionable guidance.
  Delete the comment because it's speculative and we don't have enough context to confirm if validation is actually needed.

4. pkg/query-service/app/metricsexplorer/summary.go:534

• Draft comment:
  UpdateMetricsMetadata method is simple. Consider validating the input parameters (e.g. non-empty MetricName) before calling reader.UpdateMetricsMetadata.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  Input validation is generally good practice, especially for required fields like metric names. However, I don't have enough context to know if validation is already happening elsewhere (e.g. in the request parsing layer) or if MetricName is actually required. The comment is speculative without strong evidence that this is definitely an issue.
  I don't know if validation is already happening at a higher layer. I also don't know the requirements around which fields are actually required vs optional.
  While input validation is good practice, without clear evidence that validation is missing and required, this comment is too speculative.
  Delete this comment since it makes assumptions about requirements without strong evidence that validation is actually missing and needed here.

5. pkg/query-service/app/querier/helper.go:40

• Draft comment:
  Consider refactoring the nearly duplicate implementation of prepareLogsQuery in v1 and v2 helpers to reduce code duplication.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/app/querier/v2/helper.go:22

• Draft comment:
  This prepareLogsQuery implementation is almost identical to that in v1. Consider sharing common code between v1 and v2 helpers.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/server.go:246

• Draft comment:
  Preloading updated metrics metadata: consider whether logging of each error should be aggregated or suppressed if redundant. Also ensure that blocking on PreloadMetricsMetadata does not delay startup unduly.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment raises valid concerns but they are speculative - we don't know if error logging is actually redundant or if startup is actually delayed significantly. The code change seems reasonable as is - logging each error provides maximum visibility during startup, and preloading metadata is likely an intentional startup task.
  The concerns raised could be real issues in production. Excessive logging could spam logs and startup delays could impact service availability.
  While these are valid concerns, they are speculative without evidence of actual problems. The current implementation is simple and explicit, which is good for debugging.
  Delete this comment as it is speculative and asks for verification rather than pointing out clear issues. The current implementation is reasonable unless proven problematic in production.

8. pkg/query-service/interfaces/interface.go:142

• Draft comment:
  New methods (DeleteMetricsMetadata, UpdateMetricsMetadata, GetUpdatedMetricsMetadata) have been added to the Reader interface. Confirm that these functions belong here (clickhouse-specific) according to design principles; otherwise consider moving them to the proper DAO layer.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment raises a valid concern about interface design. One of the additional rules specifically states "Do not add non-ClickHouse related functions to the ClickHouseReader interface." However, these methods seem to be general metadata operations that could be implementation-agnostic, similar to the existing GetMetricMetadata method. Without seeing the implementation, we can't be certain these are ClickHouse-specific.
  I might be wrong about the implementation-agnostic nature of these methods - they could have ClickHouse-specific details. Also, I don't have access to the actual implementation to verify.
  While the implementation details would help, the method signatures themselves look generic and similar to existing metadata methods in the interface. The interface is named Reader, not ClickHouseReader, suggesting it's meant to be implementation-agnostic.
  The comment should be deleted because we don't have strong evidence that these methods are ClickHouse-specific, and they appear to be general metadata operations consistent with the existing interface.

9. pkg/query-service/model/updatedMetricsMetadata.go:1

• Draft comment:
  The UpdateMetricsMetadata struct and its binary marshal/unmarshal methods look correct and clean.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

10. pkg/query-service/model/metrics_explorer/summary.go:164

• Draft comment:
  The new UpdateMetricsMetadataRequest definition and other metric explorer summary types look correct. Ensure field naming consistency (e.g. 'end' vs 'endD') for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. pkg/query-service/app/clickhouseReader/reader.go:5288

• Draft comment:
  Typo: Consider renaming 'ReleStateItem' to 'RuleStateItem' for clarity and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/clickhouseReader/reader.go:4890

• Draft comment:
  Typo: The variable name 'atrributeValue' should be corrected to 'attributeValue' for proper spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. pkg/query-service/app/clickhouseReader/reader.go:3240

• Draft comment:
  Typo: The SQL query string for adding an index is missing an opening backtick for the index name. It currently has %s_idx with a trailing backtick but no leading one. Ensure that the index name is wrapped in backticks correctly (e.g. "%s_idx").
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. pkg/query-service/app/http_handler.go:4896

• Draft comment:
  Typo: the variable name 'errQuriesByName' is misspelled; it should be 'errQueriesByName'. Correcting this will improve the clarity of the code.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. pkg/query-service/app/metricsexplorer/summary.go:393

• Draft comment:
  Typographical Error: Consider renaming the function 'getQueryRangeForRelateMetricsList' to 'getQueryRangeForRelatedMetricsList' for clarity and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. pkg/query-service/app/querier/helper.go:220

• Draft comment:
  Typographical error: Consider correcting 'non-existant cache key' to 'non-existent cache key' to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

17. pkg/query-service/app/querier/v2/helper.go:98

• Draft comment:
  Typo: The comment says 'sift by' but it should be 'shift by' to accurately describe the time shift applied.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. pkg/query-service/app/querier/v2/helper.go:221

• Draft comment:
  Typo: The comment uses 'non-existant' which should be corrected to 'non-existent' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

19. pkg/query-service/app/server.go:248

• Draft comment:
  Typo in error message: Consider revising "preload metrics updated metadata failed" to something clearer such as "failed to preload metrics metadata" or "preloading metrics metadata failed".
• Reason this comment was not posted:
  Marked as duplicate.

20. pkg/query-service/constants/constants.go:16

• Draft comment:
  Typo in comment: "Address to server internal services like alert manager". Consider changing 'server' to 'serve' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

21. pkg/query-service/constants/constants.go:343

• Draft comment:
  Typographical error in comment: 'indcate' should be corrected to 'indicate' to improve readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

22. pkg/query-service/interfaces/interface.go:99

• Draft comment:
  The return type in GetOverallStateTransitions is declared as []model.ReleStateItem. Is 'ReleStateItem' a typo? It looks like it might be intended to be 'RuleStateItem'. Please verify and correct if necessary.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

23. pkg/query-service/model/metrics_explorer/summary.go:19

• Draft comment:
  Typo alert: The constant 'TimeSeriesTeeMap' on line 19 appears to be a typographical error. It should likely be renamed to 'TimeSeriesTreeMap' to match the naming convention used by the rest of the code.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_X1JfjfAusbBJkbgB

---

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 0d23b80 in 1 minute and 18 seconds

More details

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

1. ee/query-service/app/server.go:305

• Draft comment:
  Good improvement on log messaging for preloading metrics metadata. The new text clearly indicates the failure context.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. pkg/query-service/app/summary.go:152

• Draft comment:
  Updated error messages now accurately reflect the API action. Ensure consistency in log texts across similar handler functions.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. pkg/query-service/constants/constants.go:89

• Draft comment:
  The cache key env variable is now more descriptive. Confirm that this change aligns with downstream cache usage and naming conventions.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. ee/query-service/app/server.go:305

• Draft comment:
  Improved log message clarity – now it clearly states 'failed to preload metrics metadata' instead of the ambiguous previous text.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. pkg/query-service/app/summary.go:152

• Draft comment:
  Updated error log messages to accurately reference the update metrics metadata operation rather than unrelated query/inspect operations.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

6. pkg/query-service/constants/constants.go:89

• Draft comment:
  The UpdatedMetricsMetadataCachePrefix constant now uses a more descriptive env var name and sensible default, improving consistency.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

7. pkg/query-service/app/summary.go:59

• Draft comment:
  The log message at line 59 reads "error getting metrics summary error"; the duplicate 'error' seems unintentional. Consider revising it to "error getting metrics summary" for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/summary.go:79

• Draft comment:
  At line 79, the error message reads "error parsing metric query range params", which seems inconsistent with the context of listing metrics. It appears to be a leftover from another function and should likely be updated to reflect the correct operation.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/app/summary.go:92

• Draft comment:
  In the GetTreeMap function, the error messages at lines 92 and 98 refer to a 'heatmap' (e.g., "error parsing heatmap metric params" and "error getting heatmap data"), but the context (and function names) suggests these should refer to a 'treemap'. Please update these messages for consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. pkg/query-service/constants/constants.go:16

• Draft comment:
  Typo in comment: 'Address to server internal services like alert manager' should be corrected to 'Address to serve internal services like alert manager'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. pkg/query-service/constants/constants.go:343

• Draft comment:
  Typo in comment: In the comment describing ReservedColumnTargetAliases, replace 'indcate' with 'indicate'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_icMHAo2KNPfJBMOd

---

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

[srikanthccv]

We have FetchTemporality that should be updated as well.

[ellipsis-dev]

❌ Changes requested. Incremental review on 88b16c5 in 4 minutes and 2 seconds

More details

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

1. pkg/query-service/app/metricsexplorer/parser.go:94

• Draft comment:
  In ParseUpdateMetricsMetadataParams, the validation of temporality for each metric type is well structured. Consider aligning the error message for histogram metrics (line 126) to mention that temporality is required when metric type is Histogram rather than Sum (current error message may be confusing).
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/metricsexplorer/summary.go:534

• Draft comment:
  In the UpdateMetricsMetadata method, the function automatically converts a Sum metric (non‑monotonic with cumulative temporality) to a Gauge. Confirm that this behavior is intended, since it changes the stored metric type based solely on temporality and monotonicity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment is asking the author to "confirm that this behavior is intended". This directly violates the rule that says "Do NOT ask the PR author to confirm their intention". The conversion appears to be an intentional design choice since it's implemented consistently in two places in the code. The comment isn't pointing out a clear issue that needs fixing.
  The conversion from Sum to Gauge could potentially be a significant behavior that affects metric types. Maybe this deserves discussion?
  While the behavior is significant, asking for confirmation isn't helpful. If there was a real issue, the reviewer should point it out directly. The consistent implementation in two places suggests this is intentional design.
  Delete the comment. It violates the rule about asking for confirmation and doesn't identify a clear issue that needs fixing.

3. pkg/query-service/app/querier/helper.go:220

• Draft comment:
  When handling metrics queries, the code fetches updated metrics metadata (lines 214-218) to update the aggregate attribute type and temporality. Ensure that GetUpdatedMetricsMetadata is consistently implemented to return correct metadata for metrics queries.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that a function is implemented correctly, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue with the code.

4. pkg/query-service/app/querier/v2/helper.go:213

• Draft comment:
  Similar to the v3 helper, the v2 querier updates the builder query's aggregate attribute with metadata. Confirm that using metricsV4.PrepareMetricQuery with metricsV3.Options as the option (line 226) is intentional and that types are compatible.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/query-service/model/updatedMetricsMetadata.go:14

• Draft comment:
  The UpdateMetricsMetadata struct properly maps fields with ClickHouse column tags. Ensure that the time format for CreatedAt meets ClickHouse requirements, and that MarshalBinary and UnmarshalBinary implementations are sufficient for caching or storage.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

6. pkg/query-service/app/querier/helper.go:221

• Draft comment:
  Remove or rephrase the informal comment ('What is happening here?') in the caching logic; it may not be needed in production code.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/querier/v2/helper.go:221

• Draft comment:
  Remove or clarify the informal comment ('What is happening here?') in the metrics caching section; similar comments appear in this file and may not be necessary.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/model/metrics_explorer/summary.go:18

• Draft comment:
  Possible typo in constant name 'TimeSeriesTeeMap'; consider renaming it to 'TimeSeriesTreeMap' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/model/metrics_explorer/summary.go:11

• Draft comment:
  Consider renaming the field 'EndD' (used in SummaryListMetricsRequest and TreeMapMetricsRequest) to 'End' for clarity, unless there is a specific reason for this naming.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. pkg/query-service/app/clickhouseReader/reader.go:6620

• Draft comment:
  Typo: 'temoporality' is misspelled. Please change it to 'temporality' to maintain consistency.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

11. pkg/query-service/app/clickhouseReader/reader.go:3930

• Draft comment:
  Typo: The error message says 'atleast 1 metric name must be specified'. Please change 'atleast' to 'at least'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/clickhouseReader/reader.go:5368

• Draft comment:
  Typo: 'ReleStateItem' appears to be misspelled. Consider renaming it to 'RuleStateItem' to improve clarity and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. pkg/query-service/app/http_handler.go:4895

• Draft comment:
  Typographical error: The variable name 'errQuriesByName' is misspelled. Please correct it to 'errQueriesByName' for clarity and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. pkg/query-service/app/metricsexplorer/parser.go:127

• Draft comment:
  Typographical error: In the histogram case, the error message refers to 'Sum' instead of 'Histogram'. Please update the message to accurately reflect that temporality is required when metric type is Histogram.
• Reason this comment was not posted:
  Marked as duplicate.

15. pkg/query-service/app/metricsexplorer/summary.go:219

• Draft comment:
  Typo: The case label 'metrics_explorer.TimeSeriesTeeMap' appears to have a typo. It should likely be 'TimeSeriesTreeMap' to match the common naming convention (as seen in 'SamplesTreeMap').
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. pkg/query-service/app/metricsexplorer/summary.go:393

• Draft comment:
  Typo/Naming issue: The helper function 'getQueryRangeForRelateMetricsList' seems to have a slight naming inconsistency. It might be clearer to rename it to 'getQueryRangeForRelatedMetricsList' to reflect its purpose more accurately.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. pkg/query-service/app/querier/helper.go:222

• Draft comment:
  Typo: In the comment, 'non-existant' should be corrected to 'non-existent' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. pkg/query-service/app/querier/v2/helper.go:99

• Draft comment:
  Typo: In the comment on line 99, 'sift by' should be corrected to 'shift by' to match the variable name and meaning.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

19. pkg/query-service/app/querier/v2/helper.go:221

• Draft comment:
  Typo: In the comment starting on line 221, 'non-existant' should be corrected to 'non-existent'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

20. pkg/query-service/model/metrics_explorer/summary.go:19

• Draft comment:
  Typo detected: 'TimeSeriesTeeMap' may be intended as 'TimeSeriesTreeMap' for consistency and clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_PsYR94Otgd0GOXTY

---

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]

In the histogram case, the error message refers to 'Sum' instead of 'Histogram'. Update the error message to state that temporality is required when metric type is Histogram.

Suggested change

	Err: fmt.Errorf("temporality is required when metric type is Sum"),
	Err: fmt.Errorf("temporality is required when metric type is Histogram"),

[ellipsis-dev]

👍 Looks good to me! Incremental review on d6f4e64 in 1 minute and 38 seconds

More details

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

1. pkg/query-service/app/metricsexplorer/parser.go:123

• Draft comment:
  Typo: In the histogram branch, the error message incorrectly refers to 'Sum' instead of 'Histogram'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/metricsexplorer/parser.go:123

• Draft comment:
  In the Histogram branch of ParseUpdateMetricsMetadataParams, the error message on empty temporality incorrectly refers to 'metric type is Sum'. It should say 'temporality is required when metric type is Histogram'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/metricsexplorer/parser.go:41

• Draft comment:
  For consistency, consider using a non-pointer variable for JSON decoding (as done in ParseRelatedMetricsParams) rather than declaring a nil pointer (e.g. in ParseSummaryListMetricsParams and ParseTreeMapMetricsParams). While decoding into a pointer-to-pointer is supported, consistent style improves readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  Typo: In the error message, 'atleast 1 metric name must be specified' should be 'at least 1 metric name must be specified'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/query-service/app/metricsexplorer/parser.go:124

• Draft comment:
  Typographical error: In the MetricTypeHistogram case, the error message wrongly states "temporality is required when metric type is Sum". It should be updated to "temporality is required when metric type is Histogram" to reflect the correct metric type.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_na3nx4OZUlQmo5L4

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 61e2cb5 in 1 minute and 12 seconds

More details

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

1. pkg/query-service/app/summary.go:72

• Draft comment:
  In ListMetrics, the error returned from ListMetricsWithSummary is stored in variable 'apiErr', but on error the handler calls RespondError with 'apiError' (from ParseSummaryListMetricsParams). Likely a bug: use the correct variable (apiErr) when reporting the error.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/summary.go:16

• Draft comment:
  Consider checking error returns from io.ReadAll to avoid silently ignoring potential read errors.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/summary.go:77

• Draft comment:
  Bug: In ListMetrics the error returned by SummaryService is stored in 'apiErr' but RespondError is called with 'apiError'. Use 'apiErr' instead.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. pkg/query-service/app/summary.go:16

• Draft comment:
  Consider handling errors from io.ReadAll rather than discarding them.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  TYPOGRAPHY: The variable 'atrributeValue' is mispelled. Please change it to 'attributeValue' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  TYPOGRAPHY: The type name 'ReleStateItem' appears in the GetOverallStateTransitions function. If this is unintended, consider renaming it to 'RuleStateItem' for consistency and clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/http_handler.go:109

• Draft comment:
  Typo in the comment: 'is registers' should be corrected to 'is registered'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/summary.go:72

• Draft comment:
  Typographical error: The log message on line 72 reads "error parsing metric list metric summary api request" which redundantly repeats 'metric'. Consider revising it to something clearer, e.g., "error parsing metrics list summary request".
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/app/summary.go:92

• Draft comment:
  Typographical error: In the GetTreeMap function, the error log on line 92 states "error parsing heatmap metric params". Since the function and parameter parser are for a treemap, consider changing it to "error parsing treemap metric params" for consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_HmziWjTSQDDxPKWw

---

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

[srikanthccv]

Does the CreatedAt as time.Time work because it is actually stored as int64 in DB?

[aniketio-ctrl]

yeah it will convert it into int as seconds

[aniketio-ctrl]

bindPositional, handles this part though i will change it

[srikanthccv]

Let's not make it mandatory to provide a description

[aniketio-ctrl]

let the client send the same one, otherwise partial metadata would be there in cache and partial in tables,

[srikanthccv]

It's possible some metrics don't have the description in the first place. Why would we reject the request, and why does the client need to send "same one". The client can send a partial payload updating the type, but in return they can't get the "description is required" where there is no description or no change in the description.

[ellipsis-dev]

👍 Looks good to me! Incremental review on bd71300 in 1 minute and 21 seconds

More details

• Looked at 82 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/metricsexplorer/parser.go:123

• Draft comment:
  For MetricTypeHistogram, update error message: change 'temporality is required when metric type is Sum' to 'temporality is required when metric type is Histogram'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/metricsexplorer/parser.go:123

• Draft comment:
  For MetricTypeHistogram the error message incorrectly refers to 'Sum'. Change it to 'temporality is required when metric type is Histogram'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/metricsexplorer/parser.go:41

• Draft comment:
  Consider using consistent variable types for JSON decoding. For example, ParseSummaryListMetricsParams declares a pointer variable (listMetricsParams *SummaryListMetricsRequest) while most other parsers declare a non-pointer variable and return its address.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  Typo: 'ReleStateItem' should probably be renamed to 'RuleStateItem' for clarity and consistency with other rule state related types.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  Typo: Change 'atleast 1 metric name must be specified' to 'at least 1 metric name must be specified' for better readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/app/metricsexplorer/parser.go:127

• Draft comment:
  Typographical error: In the v3.MetricTypeHistogram case (lines 123-129), the error message on line 127 incorrectly refers to the metric type as 'Sum'. Please update the message to 'temporality is required when metric type is Histogram'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_fC3mvmYRczvCvXrO

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 1edbb7a in 1 minute and 44 seconds

More details

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

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

• Draft comment:
  This file is extremely large and encompasses many responsibilities. Consider splitting it into multiple modules/files (e.g. separating metrics, traces, logs, TTL, etc.) to improve readability and maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  In functions like GetMinAndMaxTimestampForTraceID, dynamic SQL is built by joining traceIDs directly without parameterization. This may lead to SQL injection issues. Use parameterized queries instead.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  Multiple queries use fmt.Sprintf to embed parameters without proper parameterization. Review these to ensure user inputs are sanitized and/or use prepared statements.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  The code makes extensive use of context.WithValue (e.g. setting 'clickhouse_max_threads'). Document the intent for such context keys to aid future maintainers.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  In CheckForLabelsInMetric, the query uses JSONExtract and JSONHas with interpolated conditions. Verify that these operations safely handle client-provided label keys.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

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

• Draft comment:
  The PreloadMetricsMetadata function collects errors into a slice. Consider returning a composite error or logging warnings rather than forcing the caller to handle a list of errors.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/clickhouseReader/reader.go:4240

• Draft comment:
  There are several inline comments and commented-out code blocks. Clean these up before merging to reduce confusion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/clickhouseReader/reader.go:3500

• Draft comment:
  Error logging is very verbose across many functions. Consider centralizing error handling or using helper functions to reduce duplicated logging code.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/app/clickhouseReader/reader.go:5250

• Draft comment:
  SQL query construction via fmt.Sprintf with user-controlled values (e.g. ruleID, traceIDs, etc.) may lead to injection risks. Use parameterized queries (or prepared statements) consistently instead of interpolating strings.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. pkg/query-service/app/clickhouseReader/reader.go:5680

• Draft comment:
  Several queries concatenate slices (e.g. joining traceIDs) using strings.Join. If any of these inputs are not sanitized, this is a potential injection risk. Ensure values come from trusted sources or switch to parameter binding.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. pkg/query-service/app/clickhouseReader/reader.go:6870

• Draft comment:
  Direct use of Sprintf to build SQL queries throughout the file (e.g. in GetTraceStateHistoryByRuleID, GetUpdatedMetricsMetadata, etc.) makes it harder to guarantee safety. Refactor many of these long query strings to use proper parameter placeholders to improve security and maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/clickhouseReader/reader.go:6800

• Draft comment:
  There is a pattern of duplicating similar logic across multiple functions (for example, functions handling metrics metadata caching, attributes extraction, and live tail log queries). Consider refactoring common logic into helper functions to reduce code duplication and ease maintenance.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. pkg/query-service/app/clickhouseReader/reader.go:6900

• Draft comment:
  Some functions contain commented‐out code (e.g. dropping materialized columns) that should be removed to improve code clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. pkg/query-service/app/clickhouseReader/reader.go:4583

• Draft comment:
  The helper function getPersonalisedError checks error messages by literal string (e.g. "code: 307"). Consider defining these error codes as constants and documenting their meaning for maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. pkg/query-service/app/clickhouseReader/reader.go:4900

• Draft comment:
  Very large functions (some spanning hundreds of lines) would benefit from splitting into smaller, focused functions. This would improve readability and testability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. pkg/query-service/app/clickhouseReader/reader.go:5270

• Draft comment:
  When constructing queries that embed constant database/schema/table names, ensure that these values come from safe configuration or constants. While not a direct injection risk, it is easier to audit if these are distinct constants.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. pkg/query-service/app/clickhouseReader/reader.go:5910

• Draft comment:
  There is a mixture of using r.db.QueryRow and r.db.Select. Consistency in error handling and closing rows is important – ensure that every query invocation calls defer rows.Close() and properly passes context.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. pkg/query-service/app/clickhouseReader/reader.go:4249

• Draft comment:
  Typo: The variable 'atrributeValue' contains an extra 'r'. Consider renaming it to 'attributeValue' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

19. pkg/query-service/app/clickhouseReader/reader.go:5480

• Draft comment:
  Typo: The type 'model.ReleStateItem' appears to be misnamed. Consider renaming it to 'model.RuleStateItem' for consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

20. pkg/query-service/app/clickhouseReader/reader.go:3235

• Draft comment:
  Formatting issue: In the ALTER TABLE statement, the column name for the EXISTS flag appears to have a misplaced backtick. Ensure that the column name is properly enclosed with matching backticks (for example, use "%s_exists" instead of %s_exists).
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_q0FUtg0FjHUQ9rJx

---

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

[ellipsis-dev]

Skipped PR review on 1742638 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.