Native Histograms: support native_histogram_min_bucket_factor in scrape_config

[fatsheep9146]

fix #12864

[beorn7]

Thank you very much.

Before doing a detailed review, let's make a call about two basic ideas:

1. Configure the feature via a factor rather than a schema.
2. Combine the two wrapped appenders into one to avoid calling ReduceResolution twice.

See comments for details. I hope both are feasible. @krajorama I would like your input on this, too.

General remark: We still need to update the config docs in https://github.com/prometheus/prometheus/blob/main/docs/configuration/configuration.md , both for the new setting introduced and for the already implemented changed behavior of native_histogram_bucket_limit.

[beorn7]

@krajorama is on vacation already.

@fatsheep9146 what do you think about my comments? If you broadly agree, feel free to work on this. We can also discuss things when everyone is back from the holidays.

[fatsheep9146]

@krajorama is on vacation already.

@fatsheep9146 what do you think about my comments? If you broadly agree, feel free to work on this. We can also discuss things when everyone is back from the holidays.

I agree with your comments, I will continue working on this as you suggests!

[beorn7]

I'll review this further once I'm back from Christmas vacations.

[fatsheep9146]

I'll review this further once I'm back from Christmas vacations.

Thanks!

[krajorama]

Thank you very much.

Before doing a detailed review, let's make a call about two basic ideas:

1. Configure the feature via a factor rather than a schema.

2. Combine the two wrapped appenders into one to avoid calling `ReduceResolution` twice.

See comments for details. I hope both are feasible. @krajorama I would like your input on this, too.

I see the 2nd point is already settled.
About the 1st point , I'm torn.

Pro using factor:

• independent of technical implementation, i.e. schema.

Con using factor:

• We did use factor on client side , but only in the GO client and not the Java client. And OpenTelemetry doesn't have factor only the schema.
• I feel the min in the name can be a bit misleading as generally limits are max. And if the desired user outcome is to limit the storage, it's more natural to talk about max of something.

We might actually consider dropping factor (deprecate) in the GO client lib. Let's discuss on the next sync meeting.

[krajorama]

Thank you very much.
Before doing a detailed review, let's make a call about two basic ideas:

1. Configure the feature via a factor rather than a schema.

2. Combine the two wrapped appenders into one to avoid calling `ReduceResolution` twice.

See comments for details. I hope both are feasible. @krajorama I would like your input on this, too.

I see the 2nd point is already settled. About the 1st point , I'm torn.

Pro using factor:

* independent of technical implementation, i.e. schema.

Con using factor:

* We did use factor on client side , but only in the GO client and not the [Java client](https://prometheus.github.io/client_java/api/io/prometheus/metrics/core/metrics/Histogram.Builder.html#nativeInitialSchema(int)). And OpenTelemetry doesn't have factor only the schema.

* I feel the **min**  in the name can be a bit misleading as generally limits are **max**. And if the desired user outcome is to limit the storage, it's more natural to talk about **max** of something.

We might actually consider dropping factor (deprecate) in the GO client lib. Let's discuss on the next sync meeting.

Discussed in weekly meeting: conclusion is to use the factor, but documentation for this parameter should include a conversion table from factor to schema/scale . Schema for people familiar with Prometheus and scale for people familiar with OpenTelemetry.

[krajorama]

Please add conversion table from factor to schema/scale as explained in the general conversation.

[beorn7]

We can use round numbers in the table as we just have to hit the right band.

For example, the actual factor for schema 1 is 1.4142..., for 2 it is 1.1892, for 3 it is 1.0905..., for 4 it is 1.0443...

In the table we could say something like this:

min factor | resulting schema AKA scale
-----------+------------------
   2       |    0
   1.4     |    1
   1.1     |    2
   1.08    |    3
   1.04    |    4

[beorn7]

About the wording: I would write something like:

"Lower limit for the growth factor of one bucket to the next in each native histogram. The resolution of a histogram with a lower growth factor will be reduced until it is within the limit."

[fatsheep9146]

@beorn7 @krajorama I added the conversion table and modify the comments as you suggested.

[krajorama]

LGTM

[beorn7]

Codewise all good, thank you very much. Just some wordsmithing suggestions for the documentation, and maybe we could use a default of 0 (see comment for rationale)?

[beorn7]

This default, while it makes sense, looks very obscure to those that do not know the internals of native histograms. Could we be consistent and just use a default of 0, with the usual premise that "0 means no limit"?

[fatsheep9146]

I agreed with this expression.
Just to make sure, the 'no limit' should mean the schema should still not be larger than 8, rather than being unlimitedly large, right?
@beorn7

[beorn7]

Currently, it means schema 8, because that is the largest schema Prometheus currently supports. But we might decide to support larger schemas in the future. So 0 essentially means "lowest factor supported by Prometheus, currently that results in 1.00271".

[beorn7]

Thank you very much. Looks all great, just that I realized that a lot of the text isn't rendered as markdown and we should adjust to it. Also added a suggested wording about the default value, which should also answer your question.

[beorn7]

Let's better break this lines so that an 80 character line length is not exceeded.
These lines are rendered as they are formatted here. See how it looks on the website: https://prometheus.io/docs/prometheus/latest/configuration/configuration

[beorn7]

I have just realized that this is not rendered as markdown, but as pure text. So this table should probably get a "blank" line (just an #) to separate it from the text more clearly, and should be formatted in true ASCII-art style so that the column widths match.

[beorn7]

Let's add a line here about the default, like: "0 results in the smallest supported factor (which is currently ~1.0027 or schema 8, but might change in the future)."

[fatsheep9146]

1.00271

Thanks for your patient review, I already adjust the doc as you suggested @beorn7

[beorn7]

Thank you very much. Looks beautiful now.