feat(summary): added update metrics metadata api #7235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 487fe7f in 3 minutes and 26 seconds
More details
- Looked at
389lines of code in13files - Skipped
0files when reviewing. - Skipped posting
23drafted 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%<= threshold50%
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%<= threshold50%
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_idxwith 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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0d23b80 in 1 minute and 18 seconds
More details
- Looked at
46lines of code in3files - Skipped
0files when reviewing. - Skipped posting
11drafted 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%<= threshold50%
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%<= threshold50%
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%<= threshold50%
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%<= threshold50%
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%<= threshold50%
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%<= threshold50%
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
reviewed
Mar 7, 2025
| if err := json.NewDecoder(r.Body).Decode(&updateMetricsMetadataReq); err != nil { | ||
| return nil, &model.ApiError{Typ: model.ErrorBadData, Err: fmt.Errorf("cannot parse the request body: %v", err)} | ||
| } | ||
| return &updateMetricsMetadataReq, nil |
There was a problem hiding this comment.
Let's add validation here. The update metric type should be one of the valid metric types, same for temporality. A gauge can't have temporality. If a metric is being updated to Histogram, and if there are not buckets in the data, we should reject etc... all sensible validations.
| var allMetricsMetadata []model.UpdateMetricsMetadata | ||
| var errorList []error | ||
| // Fetch all rows from ClickHouse | ||
| query := fmt.Sprintf(`SELECT metric_name, type, description FROM %s.%s;`, signozMetricDBName, signozUpdatedMetricsMetadataTable) |
There was a problem hiding this comment.
Do we need a description in cache? Shouldn't there be temporality?
| query := fmt.Sprintf(`SELECT metric_name, type, description | ||
| FROM %s.%s | ||
| WHERE metric_name = ?;`, signozMetricDBName, signozUpdatedMetricsMetadataTable) | ||
| err := r.db.Select(ctx, query, metricName) |
There was a problem hiding this comment.
This is incomplete? What are we reading the result into and returning when there is a cache miss?
Comment on lines
6600
to
6601
| insertQuery := fmt.Sprintf(`INSERT INTO %s.%s (metric_name, type, description, created_at) | ||
| VALUES ( ?, ?, ?, ?);`, signozMetricDBName, signozUpdatedMetricsMetadataTable) |
There was a problem hiding this comment.
unit and temporality are missing?
Comment on lines
6606
to
6609
| err = r.cache.Store(ctx, constants.UpdatedMetricsMetadataCachePrefix+req.MetricName, req, -1) | ||
| if err != nil { | ||
| return &model.ApiError{Typ: "ClickHouseError", Err: err} | ||
| } |
There was a problem hiding this comment.
This is not a ClickHouseError and I don't think we should return an error to the user if the cache store fails.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 88b16c5 in 4 minutes and 2 seconds
More details
- Looked at
217lines of code in8files - Skipped
0files when reviewing. - Skipped posting
20drafted 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%<= threshold50%
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%<= threshold50%
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.
| if updateMetricsMetadataReq.Temporality == "" { | ||
| return nil, &model.ApiError{ | ||
| Typ: model.ErrorBadData, | ||
| Err: fmt.Errorf("temporality is required when metric type is Sum"), |
There was a problem hiding this comment.
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"), |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d6f4e64 in 1 minute and 38 seconds
More details
- Looked at
103lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduces an API to update metrics metadata, including server initialization, data handling, and query execution changes.
UpdateMetricsMetadataDatahandler insummary.goto update metrics metadata./api/v1/metrics/{metric_name}/metadatainhttp_handler.gofor updating metrics metadata.server.gousingPreloadMetricsMetadata().UpdateMetricsMetadataandGetUpdatedMetricsMetadatafunctions inreader.gofor ClickHouse operations.DeleteMetricsMetadatafunction inreader.goto handle metadata deletion.UpdateMetricsMetadatastruct inupdatedMetricsMetadata.gofor metadata representation.UpdateMetricsMetadataRequestinmetrics_explorer/summary.gofor request handling.runBuilderQuery()inhelper.goandv2/helper.goto use updated metrics metadata.UpdatedMetricsMetadataCachePrefixinconstants.gofor caching purposes.This description was created by
for d6f4e64. It will automatically update as commits are pushed.