feat: NHCB: convert classic histograms to nhcb in scrape MVP by krajorama · Pull Request #14978 · prometheus/prometheus

krajorama · 2024-09-25T11:40:34Z

Originally started in #14008 by @zenador

Partially addresses #13529 and #13532

MVP:
Supports any scrape protocol - I've tried with Protobuf and OpenMetrics1.0.0.
Supports multiple exemplars per histogram.
Assumes that metadata is present as it depends on that to detect classic histogram series for conversion. Thus naked histograms without # TYPE histogram are not converted.
Does not support created timestamp (--enable-feature=created-timestamp-zero-ingestion).
Does not support # TYPE gaugehistogram yet.

Before #14730 is implemented, reload from WAL does not work. Test with:

rm -rf ./data ; mkdir data; ~/go/github.com/prometheus/prometheus/prometheus --enable-feature=exemplar-storage,native-histograms

My sample config:

global:
  scrape_interval: 1s
  external_labels:
    scraped_by: prometheus
  scrape_protocols: [ OpenMetricsText1.0.0 ]

scrape_configs:
  - job_name: mimir-1
    convert_classic_histograms_to_nhcb: true
    static_configs:
      - targets: ['10.10.10.10:5001']
        labels:
          container: 'promtestsource-1'

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

…pend classic histogram series) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

… if they are not actually histograms based on metadata Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

# Conflicts: # config/config.go # scrape/scrape.go

bwplotka · 2024-10-04T07:17:30Z

Nice, let's review benchmark code #15083 and will check this PR out for review/efficiency assessment 👍🏽

# Conflicts: # scrape/scrape.go

…e name See Parser.Type() function. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Update test cases Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Renaming Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Scrape test for NHCB modified. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

On reviewer request. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

#14978 (comment) Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

bwplotka

Thanks! Only #14978 (comment)

scrape/scrape.go

Signed-off-by: George Krajcsovits <krajorama@users.noreply.github.com>

From comment #14978 (comment) Also make compareLabels read only and move storeLabels to the first processed classic histogram series. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

* NHCB: scrape use state field and not booleans From comment #14978 (comment) Also make compareLabels read only and move storeLabels to the first processed classic histogram series. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> * Speed up TestConvertClassicHistogramsToNHCB 3x Reduce the startup time and timeouts Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> * lint fix Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> --------- Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

From: #14978 (comment) Also encode the requirement table set in #13532 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

…15209) From: #14978 (comment) Also encode the requirement table set in #13532 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

zenador and others added 19 commits September 25, 2024 13:36

implement basic conversion of classic to nhcb in scrape

62e7f04

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

convert classic histograms to int nhcb where possible instead

4503145

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

improve new scrape test

0a321fe

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

don't use cache for nhcb maps

02d5abf

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

allow option to convert classic histograms to nhcb entirely (don't ap…

f596f17

…pend classic histogram series) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

insert nhcb parser as intermediate layer

172d4f2

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

keep only 1 nhcb in memory at at time

0e5072b

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

skip nhcb conversion if there is native histogram of same name

cbd5488

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

add doc comments

57bde06

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

don't reuse the buffer

41c7f7d

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

expand tests and support conversion to nhcb in the middle of scrape

cd49896

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

expand tests for protobuf and fix problems

e389918

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

expand tests for classic and exponential native histograms

8b3ae15

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

don't blindly convert series with the classic histogram name suffixes…

f35c664

… if they are not actually histograms based on metadata Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

start to cover all test cases for scrape

de9de32

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

refine test cases according to spec

050b5fc

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

fix lint

90c2668

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

use caps for NHCB

97ba2fc

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>

Merge branch 'main' into nhcb-scrape-impl

71fd2d9

# Conflicts: # config/config.go # scrape/scrape.go

krajorama mentioned this pull request Sep 25, 2024

Convert classic histograms to nhcb in scrape #14008

Closed

bwplotka self-assigned this Oct 4, 2024

bwplotka requested review from bwplotka and roidelapluie and removed request for roidelapluie October 4, 2024 07:17

krajorama added 2 commits October 7, 2024 11:23

Merge branch 'main' into nhcb-scrape-impl

ed2e7dc

# Conflicts: # scrape/scrape.go

Rename bType to bName as Type returns the binary name and not the typ…

9c4816d

…e name See Parser.Type() function. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama mentioned this pull request Oct 7, 2024

textparse: Refactored main testing utils for reusability; fixed proto Units. #15095

Merged

krajorama added 2 commits October 7, 2024 14:02

Add unit test nhcbparse_test.go

e1a7008

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Merge branch 'main' into nhcb-scrape-impl

e0444d9

krajorama added 11 commits October 21, 2024 11:00

Merge branch 'main' into nhcb-scrape-impl

8c1b5a6

Followup to #15164

482bb45

Update test cases Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Follow up #15178

70742a6

Renaming Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

More followup to #15164

a23aed5

Scrape test for NHCB modified. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Rename convert_classic_histograms to convert_classic_histograms_to_nhcb

4283ae7

On reviewer request. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Add unit test to show that current wrapper is sub-optimal

5ee0980

#14978 (comment) Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix failing benchmarks

eaee6ba

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Remove omcounterdata.txt as redundant

a6947e1

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Better docstring on test

555bd62

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

goimports run

bee1eb7

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

benchmark, rename parser omtext_with_nhcb

25ef4d3

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

bwplotka approved these changes Oct 21, 2024

View reviewed changes

bwplotka previously approved these changes Oct 21, 2024

View reviewed changes

krajorama commented Oct 21, 2024

View reviewed changes

scrape/scrape.go Outdated Show resolved Hide resolved

Update scrape/scrape.go

877fd2a

Signed-off-by: George Krajcsovits <krajorama@users.noreply.github.com>

krajorama dismissed bwplotka’s stale review via 877fd2a October 21, 2024 14:01

bwplotka approved these changes Oct 21, 2024

View reviewed changes

krajorama merged commit ad4857d into main Oct 22, 2024

krajorama deleted the nhcb-scrape-impl branch October 22, 2024 05:56

krajorama mentioned this pull request Oct 22, 2024

NHCB scrape: refactor state handling and speed up scrape test #15193

Merged

krajorama mentioned this pull request Oct 23, 2024

feat(nhcb): implement created timestamp handling #15198

Merged

krajorama added a commit that referenced this pull request Oct 24, 2024

fix(nhcb): do not return nhcb from parse if exponential is present

eb147bc

From: #14978 (comment) Also encode the requirement table set in #13532 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama mentioned this pull request Oct 24, 2024

fix(nhcb): do not return nhcb from parse if exponential is present #15209

Merged

krajorama added a commit that referenced this pull request Oct 24, 2024

fix(nhcb): do not return nhcb from parse if exponential is present

a18042a

From: #14978 (comment) Also encode the requirement table set in #13532 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama added a commit that referenced this pull request Oct 24, 2024

fix(nhcb): do not return nhcb from parse if exponential is present

8e607d9

From: #14978 (comment) Also encode the requirement table set in #13532 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama mentioned this pull request Oct 29, 2024

Redesign parser interface to return metrics and not lines #15115

Closed

julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request Dec 13, 2024

Merge pull request prometheus#14978 from prometheus/nhcb-scrape-impl

727c56d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: NHCB: convert classic histograms to nhcb in scrape MVP#14978

feat: NHCB: convert classic histograms to nhcb in scrape MVP#14978
krajorama merged 55 commits intomainfrom
nhcb-scrape-impl

krajorama commented Sep 25, 2024 •

edited

Loading

Uh oh!

bwplotka commented Oct 4, 2024

Uh oh!

bwplotka left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

krajorama commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka commented Oct 4, 2024

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krajorama commented Sep 25, 2024 •

edited

Loading