Skip to content

Remove obsolete check preventing NHCB and CT zero ingestion#17305

Merged
krajorama merged 9 commits intoprometheus:mainfrom
hxrshxz:fix-17224-remove-nhcb-check
Oct 13, 2025
Merged

Remove obsolete check preventing NHCB and CT zero ingestion#17305
krajorama merged 9 commits intoprometheus:mainfrom
hxrshxz:fix-17224-remove-nhcb-check

Conversation

@hxrshxz
Copy link
Contributor

@hxrshxz hxrshxz commented Oct 8, 2025

Which issue(s) does the PR fix:

Fixes #17216

Does this PR introduce a user-facing change?

[ENHANCEMENT] allow simultaneous use of classic histogram → NHCB conversion and zero-timestamp ingestion

Copilot AI review requested due to automatic review settings October 8, 2025 03:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes an obsolete check that prevented the simultaneous use of Native Histograms with Custom Buckets (NHCB) and created timestamp zero ingestion. The check was a temporary workaround for issue #15137 which has apparently been resolved.

  • Removed the conditional check that logged an error and skipped configuration when both NHCB and created timestamp zero ingestion were enabled
  • Eliminated the TODO comment referencing the GitHub issue

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@hxrshxz hxrshxz requested a review from Copilot October 8, 2025 03:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@hxrshxz hxrshxz requested a review from Copilot October 8, 2025 03:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from 49ba045 to ba0f464 Compare October 8, 2025 03:09
Signed-off-by: Harsh <harshmastic@gmail.com>
@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from ba0f464 to c6793e7 Compare October 8, 2025 03:11
@hxrshxz hxrshxz closed this Oct 8, 2025
@hxrshxz hxrshxz reopened this Oct 8, 2025
…sage

Signed-off-by: Harsh <harshmastic@gmail.com>
@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from 5d9d5fd to febd734 Compare October 8, 2025 03:37
@hxrshxz
Copy link
Contributor Author

hxrshxz commented Oct 8, 2025

@roidelapluie Please take a look

@krajorama krajorama self-requested a review October 11, 2025 07:58
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I've looked at the original issue in #15137 and it also complained about exemplars and OpenMetrics exposition (not protobuf) .
Would you mind modifying this test to have a few exemplars as well?

# HELP something Histogram with _created between buckets and summary
# TYPE something histogram
something_count 18
something_sum 324789.4
something_created 1520430001
something_bucket{le="0.0"} 1
something_bucket{le="+Inf"} 18 8 # {id="something-bucket-a-test"} 4
something_count{a="b"} 9
something_sum{a="b"} 42123.0
something_bucket{a="b",le="0.0"} 8 # {id="something-bucket-b-test"} -4
something_bucket{a="b",le="+Inf"} 9 # {id="something-bucket-b-test"} 6
something_created{a="b"} 1520430002

Diff:

diff --git a/model/textparse/nhcbparse_test.go b/model/textparse/nhcbparse_test.go
index f5836b5f7..61671d306 100644
--- a/model/textparse/nhcbparse_test.go
+++ b/model/textparse/nhcbparse_test.go
@@ -96,11 +96,11 @@ something_count 18
 something_sum 324789.4
 something_created 1520430001
 something_bucket{le="0.0"} 1
-something_bucket{le="+Inf"} 18
+something_bucket{le="+Inf"} 18 8 # {id="something-bucket-a-test"} 4
 something_count{a="b"} 9
 something_sum{a="b"} 42123.0
-something_bucket{a="b",le="0.0"} 8
-something_bucket{a="b",le="+Inf"} 9
+something_bucket{a="b",le="0.0"} 8 # {id="something-bucket-b-test"} -4
+something_bucket{a="b",le="+Inf"} 9 # {id="something-bucket-b-test"} 6
 something_created{a="b"} 1520430002
 # HELP yum Summary with _created between sum and quantiles
 # TYPE yum summary
@@ -368,6 +368,7 @@ foobar{quantile="0.99"} 150.1`
                                CustomValues:    []float64{0.0}, // We do not store the +Inf boundary.
                        },
                        lset: labels.FromStrings("__name__", "something"),
+                       es:   []exemplar.Exemplar{{Labels: labels.FromStrings("id", "something-bucket-a-test"), Value: 4}},
                        ct:   1520430001000,
                }, {
                        m: `something{a="b"}`,
@@ -380,6 +381,10 @@ foobar{quantile="0.99"} 150.1`
                                CustomValues:    []float64{0.0}, // We do not store the +Inf boundary.
                        },
                        lset: labels.FromStrings("__name__", "something", "a", "b"),
+                       es:   []exemplar.Exemplar{
+                               {Labels: labels.FromStrings("id", "something-bucket-b-test"), Value: -4},
+                               {Labels: labels.FromStrings("id", "something-bucket-b-test"), Value: 6},
+                       },
                        ct:   1520430002000,
                }, {
                        m:    "yum",

@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from 04c8d08 to 3a7a8d7 Compare October 11, 2025 11:57
@hxrshxz hxrshxz requested a review from krajorama October 11, 2025 12:01
@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from b875227 to ae2d5a3 Compare October 12, 2025 15:58
Signed-off-by: Harsh <harshmastic@gmail.com>
@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from ae2d5a3 to a63414b Compare October 12, 2025 15:59
@hxrshxz hxrshxz requested a review from krajorama October 12, 2025 16:13
hxrshxz and others added 2 commits October 13, 2025 00:50
Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com>
Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com>
@hxrshxz hxrshxz requested a review from krajorama October 12, 2025 19:21
@krajorama
Copy link
Member

Please remove those extra redundant docstrings from the end of the test and I'll merge this. see #17305 (comment)

Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com>
@hxrshxz
Copy link
Contributor Author

hxrshxz commented Oct 13, 2025

Please remove those extra redundant docstrings from the end of the test and I'll merge this. see #17305 (comment)

oops must have missed it

Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. I'll merge when you make CI pass.

Signed-off-by: Harsh <harshmastic@gmail.com>
@hxrshxz hxrshxz force-pushed the fix-17224-remove-nhcb-check branch from 6f551d7 to 4d7d8eb Compare October 13, 2025 08:29
@hxrshxz hxrshxz requested a review from krajorama October 13, 2025 08:46
@krajorama krajorama merged commit edbc5cf into prometheus:main Oct 13, 2025
28 checks passed
@hxrshxz hxrshxz deleted the fix-17224-remove-nhcb-check branch October 13, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove obsolete check preventing simultaneous NHCB conversion and created timestamp zero ingestion

3 participants