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: added simultaneous temporality changes | 6175 #6915

Closed
wants to merge 1 commit into 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

Add support for handling metrics with changing temporalities by detecting switch points and adjusting queries accordingly.

  • Behavior:
    • Added GetTemporalitySwitchPoints() in reader.go to fetch points where temporality changes for a metric.
    • Updated runBuilderQueries() in querier.go to handle metrics with multiple temporalities by splitting queries at switch points.
    • Modified PopulateTemporality() in http_handler.go to set MultipleTemporalities flag in BuilderQuery if temporality changes are detected.
  • Models:
    • Added TemporalityChangePoint struct in v3.go to represent temporality switch points.
    • Updated BuilderQuery struct in v3.go to include MultipleTemporalities flag.
  • Interfaces:
    • Added GetTemporalitySwitchPoints() to Reader interface in interface.go.

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

👍 Looks good to me! Reviewed everything up to bdd3b5d in 3 minutes and 52 seconds

More details
  • Looked at 246 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pkg/query-service/app/querier/v2/querier.go:237
  • Draft comment:
    The variable i should be replaced with idx inside the goroutine to avoid race conditions.
		if idx == 0 {
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. pkg/query-service/app/querier/v2/querier.go:240
  • Draft comment:
    The variable i should be replaced with idx inside the goroutine to avoid race conditions.
			if idx == 0 {
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/query-service/app/querier/v2/querier.go:248
  • Draft comment:
    The variable i should be replaced with idx inside the goroutine to avoid race conditions.
			} else if idx == len(temporalitySwitches) {
  • Reason this comment was not posted:
    Marked as duplicate.
4. pkg/query-service/app/querier/v2/querier.go:249
  • Draft comment:
    The variable i should be replaced with idx inside the goroutine to avoid race conditions.
				} else if idx == len(temporalitySwitches) {
  • Reason this comment was not posted:
    Marked as duplicate.
5. 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. First, this is a generic Reader interface, not specifically a ClickHouseReader. Second, temporality is a general concept in metrics, not specific to any database. Third, without seeing the implementation, we can't know if this is ClickHouse-related or not. The interface should define what operations are needed, not how they're implemented.
    I could be wrong about the interface's intended purpose - maybe it is meant to be ClickHouse-specific despite its generic name. Also, temporality tracking might actually be implemented in a separate DAO.
    Even if this interface is ClickHouse-specific in practice, the comment is still making assumptions about implementation details that aren't visible in this diff. The interface should focus on what operations are needed, not where they're implemented.
    The comment should be deleted as it makes assumptions about implementation details we can't verify, and appears to misunderstand the purpose of the interface abstraction.

Workflow ID: wflow_DvyxVjPHxrcToU7r


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

@aniketio-ctrl aniketio-ctrl deleted the fix/6175 branch January 24, 2025 03:39
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.

1 participant