Add support for native histograms in OM parser by vesari · Pull Request #1040 · prometheus/client_python · GitHub

This PR adds an OM parser for native histograms, as a first step towards the implementation of the following proposal prometheus/proposals#32 . The next steps (as far as this repo is concerned) would be the OM exposition and then obviously the implementation of native histograms generation/writing logic.

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

…abel set Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Just a quick read through, but I was thinking maybe we limit the changes in the initial PR to just the parser changes? We could get that in and start iterating on it, and it will then be useful for testing the exposition changes.

I believe the internal code should create the schema, and may even change the schema value on occassion.

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Just a quick read through, but I was thinking maybe we limit the changes in the initial PR to just the parser changes? We could get that in and start iterating on it, and it will then be useful for testing the exposition changes.

I totally agree!

…nd adapt logic accordigly Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Nice! I left a handful of comments, they probably don't all need to be addressed in this PR, but could be refactorings in future PRs as well.

Mainly I would like to see as few changes to other files, and especially function signatures, as possible. Even if that means we do some things like skipping values that are histograms for now. That way hopefully no one depends on some code/type changes we might update soon.

Graphite doesn't have support for native histograms, if for some reason one shows up in a registry we would probably need to drop it with a warning. For now I would say let's not make any changes here as native histograms will be experimental anyway.

You're absolutely right. The thing is the mypy linter fails without this change. That's due to the native histogram value being a union. I noticed that only when I pushed the PR for the first time

Solved after separating fields

I would avoid the else block and move these into the try block. That keeps all the happy path/text parsing code together.

Does this need to change yet? It would be nice to have no/minimal public interface changes since we won't be using these in the registry quite yet.

Same as for the Graphite bridge file. mypy linting fails otherwise.

Solved after separating fields

Trying to decide if this should be a union or if we should have a separate field for histogram_value or something like that.

Given the union's "side-effects" in files like registry etc, maybe it should be a separate field? My initial thinking with the union was to possibly avoid function signature changes I guess, but maybe it wouldn't be that problematic?

Yeah, let's try having a separate Optional field for the native histogram and see what it looks like. It also makes it nice to just check if the value is none or not for if a value is a native histogram.

I separated them, and added a field for native histogram. I added it for last to minimize the risk of breaking something.

Rather than nesting everything further in what about a continue if len(suffix) is 0?

Let's remove these commented out lines until they are actually used.

Thanks for the suggested changes, I'll be working on it :)

Just a thought, we could probably just call this NativeHistogram rather than adding StructValue. For initial implementation we might want to prefix with _ as well to these new types to make it clear they are internal only and may change. Or add a comment to that effect.

…locks Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Just a couple of nits/removals and then I think this iteration is ready to merge! Thanks a bunch!

Nit, could you just remove this line so we don't have a needless diff/history entry?

Done!

🤔 Not sure if we need this here at all (it will need to be in metrics.py). This function is for custom collectors, and I think if someone is creating a native histogram for that case they will end up needing to define all the spans themself. For now I would say just leave it out.

Yes, I deleted it.

We should remove the debug printing before merging, there are a couple other lines in this function as well.

I could see just another one, I hope I removed them all now XD

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Thanks for all the work on this!

vesari added 7 commits June 15, 2024 18:12

Start on native histogram parser

977b0b2

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Fix regex for nh sample

fd1b563

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Get nh sample appended

e32d2a8

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Complete parsing for simple native histogram

cb013d8

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Add parsing for native histograms with labels, fix linting

4b1f527

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Mitigate type and style errors

eb6d9de

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Add test for parsing coexisting native and classic hist with simple l…

86f165a

…abel set Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

csmarchbanks reviewed Jul 3, 2024

View reviewed changes

Solve error in Python 3.9 tests

c69a500

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Add test for native + classic histograms with more than a label set a…

c06db3f

…nd adapt logic accordigly Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

vesari marked this pull request as ready for review July 8, 2024 13:17

vesari requested a review from csmarchbanks July 8, 2024 13:17

vesari changed the title WIP: Native histogram support Add support for native histograms in OM parser Jul 9, 2024

csmarchbanks reviewed Jul 11, 2024

View reviewed changes

csmarchbanks reviewed Jul 17, 2024

View reviewed changes

Separate native histogram from value field, improve conditional/try b…

d394c71

…locks Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

csmarchbanks reviewed Aug 30, 2024

View reviewed changes

Clean up debug lines, add warnings, delete unnecessary lines

90cd08e

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

csmarchbanks approved these changes Sep 20, 2024

View reviewed changes

csmarchbanks merged commit d7c9cd8 into prometheus:master Sep 20, 2024

beorn7 mentioned this pull request Dec 11, 2024

OM 2.0: Native Histogram Support in Text format. prometheus/OpenMetrics#279

Closed

vesari mentioned this pull request Feb 12, 2025

OM text exposition for NH #1087

Merged

                                registry: Optional[CollectorRegistry] = REGISTRY,
                                _labelvalues: Optional[Sequence[str]] = None,
                                buckets: Sequence[Union[float, str]] = DEFAULT_BUCKETS,
+                               # native_hist_schema: Optional[int] = None, # create this dynamically?

+                  else:
+                      elems = pos_spans_text.split(',')
+                      arg1 = [int(x) for x in elems[0].split(':')]
+                      arg2 = [int(x) for x in elems[1].split(':')]
+                      pos_spans = (BucketSpan(arg1[0], arg1[1]), BucketSpan(arg2[0], arg2[1]))

                   name: str
                   labels: Dict[str, str]
-                  value: float
+                  value: Union[float, NativeHistStructValue]

-                          has_gsum = True
-                          if s.value < 0:
-                              has_negative_gsum = True
+                      if len(suffix) != 0:

+                               # native_hist_schema: Optional[int] = None,
+                               # native_hist_bucket_fact: Optional[float] = None,
+                               # native_hist_zero_threshold: Optional[float] = None,
+                               # native_hist_max_bucket_num: Optional[int] = None,
+                               # native_hist_min_reset_dur: Optional[timedelta] = None,
+                               # native_hist_max_zero_threshold: Optional[float] = None,
+                               # native_hist_max_exemplars: Optional[int] = None,
+                               # native_hist_exemplar_TTL: Optional[timedelta] = None,

+                              # using a safe float convert on s.value as a temporary workaround while figuring out what to do
+                              # in case value is a native histogram structured value, if that's ever a possibility
+                              output.append(f'{prefixstr}{_sanitize(s.name)}{labelstr} {safe_float_convert(s.value)} {now}\n')

                       return m
-                  def get_sample_value(self, name: str, labels: Optional[Dict[str, str]] = None) -> Optional[float]:
+                  def get_sample_value(self, name: str, labels: Optional[Dict[str, str]] = None) -> Optional[Union[float, NativeHistStructValue]]:

                       t = _RegularPush(self, interval, prefix)
                       t.daemon = True
                       t.start()

                                sum_value: Optional[float] = None,
                                labels: Optional[Sequence[str]] = None,
                                unit: str = '',
+                               native_hist_bucket_factor: Optional[float] = None

+                  # check if it's a native histogram with labels
+                  re_nh_without_labels = re.compile(r'^[^{} ]+ {[^{}]+}$')
+                  re_nh_with_labels = re.compile(r'[^{} ]+{[^{}]+} {[^{}]+}$')
+                  print('we are matching \'{}\''.format(text))

		length: int


		class NativeHistStructValue(NamedTuple):

Add support for native histograms in OM parser by vesari · Pull Request #1040 · prometheus/client_python · GitHub

Uh oh!

Add support for native histograms in OM parser - #1040

Add support for native histograms in OM parser#1040csmarchbanks merged 11 commits intoprometheus:masterprometheus/client_python:masterfrom vesari:native-histogram-supportvesari/client_python:native-histogram-supportCopy head branch name to clipboard

Conversation

vesari commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vesari commented Jul 7, 2024

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vesari commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

csmarchbanks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add support for native histograms in OM parser#1040
csmarchbanks merged 11 commits into
prometheus:masterfrom
vesari:native-histogram-support

vesari commented Jun 18, 2024 •
edited
Loading