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: add support for change in temporality for counter metrics #6919

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

Conversation

aniketio-ctrl
Copy link
Contributor

@aniketio-ctrl aniketio-ctrl commented Jan 24, 2025

Summary
Added support for cumulative and delta metrics counter, for metrics in which user switched the temporality in between

Related Issues / PR's
#6175


Important

Added support for handling metrics with changing temporalities by identifying switch points and adjusting queries accordingly.

  • Behavior:
    • Added GetTemporalitySwitchPoints in reader.go to identify points where temporality changes for a metric.
    • Updated runBuilderQueries in querier.go to handle metrics with multiple temporalities by splitting queries at temporality switch points.
  • API Changes:
    • Modified PopulateTemporality in http_handler.go to set MultipleTemporalities flag when a metric has more than one temporality.
  • Models:
    • Added TemporalityChangePoint struct in v3.go to represent a change in temporality.
  • Interfaces:
    • Added GetTemporalitySwitchPoints to Reader interface in interface.go.

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

@github-actions github-actions bot added docs required enhancement New feature or request labels Jan 24, 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.

❌ Changes requested. Reviewed everything up to a449698 in 57 seconds

More details
  • Looked at 246 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. pkg/query-service/interfaces/interface.go:119
  • 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.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment seems to be making incorrect assumptions. The interface is named "Reader" not "ClickHouseReader", suggesting it's meant to be a general interface. The comment references implementation details (DAO in telemetry instance) which may not be relevant to this interface definition. Without seeing the implementation or other context, we can't be sure this function doesn't belong here.
    I could be wrong about the interface's intended purpose - maybe it is actually meant to be ClickHouse-specific despite the generic name. The comment could be referencing important architectural decisions that aren't visible in this file.
    Even if this is meant to be ClickHouse-specific, the comment makes assumptions about implementation details that aren't evident in the interface definition. The interface should define what operations are possible, not how they're implemented.
    The comment should be deleted as it makes assumptions about implementation details that aren't evident from the interface definition alone, and appears to misunderstand the generic nature of the Reader interface.

Workflow ID: wflow_d7CHNA2nhSfeMfqU


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.

@@ -807,6 +807,7 @@ func (m *MetricValueFilter) Clone() *MetricValueFilter {
}

type BuilderQuery struct {
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolved merge conflict marker found. Please resolve the conflict before merging.

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 c648b72 in 35 seconds

More details
  • Looked at 56 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. pkg/query-service/model/v3/v3.go:837
  • Draft comment:
    The MultipleTemporalities field is added but not used anywhere in the code. Ensure this field is necessary and consider implementing its usage or removing it if not needed.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. pkg/query-service/model/v3/v3.go:809
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_LTwuKDOConNjJB3e


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

@srikanthccv srikanthccv self-requested a review January 24, 2025 10:19
@srikanthccv srikanthccv changed the title feat: added simultaneous temporality changes | 6175 feat: add support for change in temporality for counter metrics Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants