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

vesari · 2024-06-18T15:59:01Z

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>

csmarchbanks

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.

csmarchbanks · 2024-07-03T20:36:23Z

 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?


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>

vesari · 2024-07-07T14:16:48Z

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>

csmarchbanks

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.

csmarchbanks · 2024-07-11T17:30:52Z

+ # 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')


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

csmarchbanks · 2024-07-11T18:43:39Z

+ 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]))


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

csmarchbanks · 2024-07-11T18:47:16Z

 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]]:


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

csmarchbanks · 2024-07-11T18:48:19Z

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


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.

csmarchbanks · 2024-07-11T18:50:44Z

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


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

csmarchbanks · 2024-07-11T18:54:43Z

+ # 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,


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

vesari · 2024-07-15T16:13:17Z

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

csmarchbanks · 2024-07-17T20:40:42Z

+ length: int
+
+
+class NativeHistStructValue(NamedTuple):


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>

csmarchbanks

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

csmarchbanks · 2024-08-30T19:19:40Z

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


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

csmarchbanks · 2024-08-30T19:26:16Z

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


🤔 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.

csmarchbanks · 2024-08-30T19:34:46Z

+ # 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))


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>

csmarchbanks

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 parserJul 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:masterSep 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

Uh oh!

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

vesari commented Jun 18, 2024•
edited
Loading