feat: added simultaneous temporality changes | 6175

[aniketio-ctrl]

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 temporality changes by identifying switch points and adjusting query execution accordingly.

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

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

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to b581657 in 1 minute and 14 seconds

More details

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

1. pkg/query-service/app/querier/v2/querier.go:237

• Draft comment:
  The loop variable i is used inside the goroutine, which can lead to incorrect behavior due to variable capture. Use a local variable to capture the loop variable.

		go func(idx int) {

• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. pkg/query-service/app/querier/v2/querier.go:182

• Draft comment:
  The loop variable queryName is used inside the goroutine, which can lead to incorrect behavior due to variable capture. Use a local variable to capture the loop variable.

go func(queryName string) {

• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Variable capture in goroutines is a common Go pitfall. When a loop variable is used in a goroutine, all goroutines can end up seeing the last value of the loop variable. However, in this case, the goroutine is launched inside the loop iteration and only uses the current value of queryName. There's no risk of the loop continuing and changing queryName's value before it's used.
  Could I be missing something about the scope of the variable? Could there be edge cases where the loop continues before the goroutine executes?
  No, this is a false positive. The goroutine is launched immediately and uses queryName right away. The loop won't continue because of the if condition at line 178 that ensures we only process when queryName matches builderQuery.Expression.
  This comment should be deleted. The variable capture issue doesn't exist here because the goroutine is launched and uses queryName within the same loop iteration, and the loop won't continue due to the if condition.

3. pkg/query-service/interfaces/interface.go:117

• 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 based on incorrect assumptions. First, this is not a ClickHouseReader interface - it's just called Reader. Second, the interface already contains many functions that aren't strictly ClickHouse-specific. Third, temporality tracking is a legitimate metrics-related function that fits with the other metrics capabilities in this interface.
  I could be wrong about the implementation details - maybe this is actually meant to be ClickHouse-specific despite the name. Also, temporality tracking might have special requirements I'm not aware of.
  Even if this is meant to be ClickHouse-specific, the interface already contains many non-ClickHouse specific functions, so this comment would be too late to enforce that design principle. The function fits the existing pattern.
  The comment should be deleted because it appears to be based on incorrect assumptions about the interface's purpose and scope.

Workflow ID: wflow_YmyaFiBKyHP2dJkx

---

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

[ellipsis-dev]

Skipped PR review on 95adea8 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.