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

feat: 7074 | added summary view apis #7075

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: 7074 | added summary view apis #7075

wants to merge 3 commits into from

Conversation

aniketio-ctrl
Copy link
Contributor

@aniketio-ctrl aniketio-ctrl commented Feb 10, 2025

Summary

Added Summary view apis
description - https://www.notion.so/signoz/API-Documentation-Metrics-Explorer-18afcc6bcd19808ea646ce51b1c7ac69?showMoveTo=true&saveParent=true

Related Issues / PR's

(#7074)


Important

Added new summary view APIs for metrics explorer, including routes, handlers, services, models, and utilities for handling metrics data.

  • APIs:
    • Added MetricExplorerRoutes in api.go to handle new summary view APIs.
    • New endpoints for metrics, treemap, metric details, filter keys, and filter values.
  • Handlers:
    • Implemented FilterKeysSuggestion, FilterValuesSuggestion, ListMetrics, GetMetricsDetails, and GetTreeMap in summary.go.
  • Services:
    • Introduced SummaryService in summary.go for handling metrics summary logic.
    • Methods for filtering keys/values, listing metrics, and getting treemap data.
  • Models:
    • Added models in summary.go for SummaryListMetricsRequest, TreeMapMetricsRequest, MetricDetail, and others.
  • Utilities:
    • Added BuildFilterConditions in filter_conditions.go for constructing filter conditions.
    • Added parsing functions in parser.go for request bodies related to metrics summary.
  • Misc:
    • Integrated SummaryService into APIHandler in http_handler.go.

This description was created by Ellipsis for 8e674c6. 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.

❌ Changes requested. Reviewed everything up to 24c59c6 in 3 minutes and 42 seconds

More details
  • Looked at 1531 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. pkg/query-service/app/metricsexplorer/summary.go:107
  • Draft comment:
    Ensure that error propagation in GetMetricsSummary (using channel errCh) works as intended when no goroutine writes an error. Since errCh is closed then reading returns nil, but consider documenting this behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 50% <= threshold 50%
    This comment is asking the author to ensure that a specific behavior is intended, which violates the rule against asking for confirmation of intended behavior. However, it also suggests documenting the behavior, which is a specific and actionable suggestion. The comment is partially useful because it provides a suggestion for improvement, but it also includes a part that violates the rules.
2. pkg/query-service/app/metricsexplorer/summary.go:149
  • Draft comment:
    In goroutine for GetAttributesForMetricName, consider logging errors from json.Marshal and Unmarshal instead of printing to stdout.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/query-service/app/metricsexplorer/parser.go:17
  • Draft comment:
    Parsing request body into a pointer variable; ensure the client sends a JSON object. Consider checking for nil input.
  • Reason this comment was not posted:
    Marked as duplicate.
4. pkg/query-service/utils/filter_conditions.go:25
  • Draft comment:
    Ensure that the helper ClickHouseFormattedValue is defined and handles quoting/escaping properly to avoid SQL injection risks.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the author to ensure that a helper function is defined and handles a specific task properly. It is not a direct code suggestion or a specific request for a test, and it falls under the rule of not asking the author to ensure behavior is intended or tested.
5. pkg/query-service/app/metricsexplorer/summary.go:145
  • Draft comment:
    Multiple goroutines assign to the same field 'LastReceived' (e.g. L147 and L163) concurrently. This can cause a race. Consider synchronizing updates or combining the results after wait.
  • Reason this comment was not posted:
    Marked as duplicate.
6. pkg/query-service/utils/filter_conditions.go:28
  • Draft comment:
    The function ClickHouseFormattedValue (L29) is used but not defined or imported. Please ensure its implementation is available.
  • Reason this comment was not posted:
    Marked as duplicate.
7. pkg/query-service/app/metricsexplorer/summary.go:200
  • Draft comment:
    Using fmt.Printf for error logging (e.g. L203 and L211) is inconsistent with zap logging used elsewhere. Consider using zap for consistency.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_v52vDN765EJVsThh


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.

return filterValueRequest, nil
}

func ParseTreeMapMetricsParams(r *http.Request) (*metrics_explorer.TreeMapMetricsRequest, *model.ApiError) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider creating a generic parser function to handle all request body parsing instead of duplicating the parsing logic.

  • function ParseSummaryListMetricsParams (parser.go)

break
}

// Add missing range if there's a gap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‘min’ and ‘max’ are used (e.g. L70, L74) but not defined or imported. Please add helper functions or import them.

@github-actions github-actions bot added enhancement New feature or request and removed testing-deploy labels Feb 12, 2025
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! Incremental review on 8e674c6 in 1 minute and 10 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/interfaces/interface.go:7
  • Draft comment:
    Removed the unused 'clickhouse-go' import. This cleans up dependencies and avoids unnecessary coupling.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it only states that an unused import was removed. It doesn't provide any actionable feedback or suggestions for improvement.
2. pkg/query-service/interfaces/interface.go:7
  • Draft comment:
    Remove the unused import of 'github.com/ClickHouse/clickhouse-go/v2' to clean up dependencies.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_nNftdO4zvB5f0Hah


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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants