Skip to content

Parse created timestamps from OpenMetrics-Text format #13506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ArthurSens wants to merge 1 commit into from
Closed

Parse created timestamps from OpenMetrics-Text format #13506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/prometheus/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func (c *flagConfig) setFeatureListOptions(logger log.Logger) error {
// Change relevant global variables. Hacky, but it's hard to pass a new option or default to unmarshallers.
config.DefaultConfig.GlobalConfig.ScrapeProtocols = config.DefaultProtoFirstScrapeProtocols
config.DefaultGlobalConfig.ScrapeProtocols = config.DefaultProtoFirstScrapeProtocols
level.Info(logger).Log("msg", "Experimental created timestamp zero ingestion enabled. Changed default scrape_protocols to prefer PrometheusProto format.", "global.scrape_protocols", fmt.Sprintf("%v", config.DefaultGlobalConfig.ScrapeProtocols))
level.Info(logger).Log("msg", "Experimental created timestamp zero ingestion enabled. Changed default scrape_protocols to prefer PrometheusProto format since it has better performance.", "global.scrape_protocols", fmt.Sprintf("%v", config.DefaultGlobalConfig.ScrapeProtocols))
case "":
continue
case "promql-at-modifier", "promql-negative-offset":
Expand Down
7 changes: 6 additions & 1 deletion model/textparse/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func New(b []byte, contentType string, parseClassicHistograms bool, st *labels.S
return NewPromParser(b, st), nil
}

mediaType, _, err := mime.ParseMediaType(contentType)
mediaType, err := ParseMediaType(contentType)
if err != nil {
return NewPromParser(b, st), err
}
Expand All @@ -99,6 +99,11 @@ func New(b []byte, contentType string, parseClassicHistograms bool, st *labels.S
}
}

func ParseMediaType(contentType string) (string, error) {
mediaType, _, err := mime.ParseMediaType(contentType)
return mediaType, err
}

// Entry represents the type of a parsed entry.
type Entry int

Expand Down
23 changes: 21 additions & 2 deletions model/textparse/openmetricsparse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,28 @@ func (p *OpenMetricsParser) Exemplar(e *exemplar.Exemplar) bool {
return true
}

// CreatedTimestamp returns nil as it's not implemented yet.
// TODO(bwplotka): https://github.com/prometheus/prometheus/issues/12980
// For OpenMetrics text, the created timestamp is exposed as it was an extra metric.
// The difference between an usual metric and the created timestamp is that for
// the created timestamp, the metric name ends with "_created".
//
// Created timestamps are always exposed right after the metric they are
Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

right after

Is that part of the OpenMetrics spec? I think in practice I have only seen _created as the last metric in a MetricFamily, but I haven't read anything in the spec that requires that. It does have to be with the other samples, but e.g. _created before _total might be possible?

Copy link
Copy Markdown
Member Author

@ArthurSens ArthurSens Feb 7, 2024

Choose a reason for hiding this comment

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

Looking at the example in their specification, I see _created lines for each metric point and not MetricFamily.

In the section Overall Structure, we have one created line for acme_http_router_request_seconds_created{path="/api/v1",method="GET"} and another for acme_http_router_request_seconds_created{path="/api/v2",method="POST"} (notice the label difference while using the same name)

but e.g. _created before _total might be possible

Do you mean for the same MetricPoint? All the examples I've seen have the _created line as the last line of a MetricPoint, but I couldn't find anything in the spec saying that this order is a MUST. I'm not sure if we should expect something different 🤔

Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks Feb 7, 2024

Choose a reason for hiding this comment

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

You're correct, I meant MetricPoint, sorry!

I agree that all of the samples show _created as the last line, but that doesn't seem enforced so I think it is possible and valid OpenMetrics for _created to come first.

Copy link
Copy Markdown
Member Author

@ArthurSens ArthurSens Feb 7, 2024

Choose a reason for hiding this comment

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

😭😭😭😭

That makes things so much more complicated 🥲

Copy link
Copy Markdown
Member

@csmarchbanks csmarchbanks Feb 7, 2024

Choose a reason for hiding this comment

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

Yeah, it might be worth getting an opinion from someone more involved in OpenMetrics than myself as I agree, it would be nice not to have to worry about the order!

Copy link
Copy Markdown
Member

@bboreham bboreham Feb 28, 2024

Choose a reason for hiding this comment

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

I'm not more involved in OpenMetrics, but I think it's totally fine that an experimental feature relies on this accidental property.

Copy link
Copy Markdown
Member

@bwplotka bwplotka Apr 5, 2024

Choose a reason for hiding this comment

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

Yea, the order of _created is not guaranteed to be the last. But also I don't understand you worries @ArthurSens -- if it's last we already have the problem of having to "buffer" previous samples related to the _created metric which is potentially at the end. So it does not matter if it's end, middle or first. We still have to buffer samples (and corresponding series, which should be the same) no matter what, so we can add potential 0 sample OR in future add it to metadata.

Right now CreatedTimestamp() returns number ONLY when parser hits _created metric, which is clearly not enough for OM as we already written sample for related series no? We have to change bit this logic to e.g. buffer series in (OpenMetrics parsing only).

To me that means e.g. having different append function all together for OM, and that's fine. It should motivate, heavy changes to newer OM protocol to fix this.

Copy link
Copy Markdown
Member Author

@ArthurSens ArthurSens Apr 5, 2024

Choose a reason for hiding this comment

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

I'm not 100% sure yet(doing research at the moment), but I believe all official client libraries that offer OpenMetrics support today expose _created lines after the related metric. If I can confirm this, and seeing that this feature is experimental, could we rely on this order for this PR?

Expecting different order for the _created lines will make the code a lot more complex, we could make reviews easier if we split this up in different work streams :)

Copy link
Copy Markdown
Member

@bwplotka bwplotka Apr 9, 2024

Choose a reason for hiding this comment

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

We can, but my point stays Arthur, how it makes the code more complex if it's before , in the middle or after the line? The main challenge is that you have to keep buffering samples somewhere BEFORE appending until you know that _created line. This is because we try to be lean and stream parsing line by line.

This is the main complexity to the code, not searching where that line is or something 🤔 I also don't see that buffering in this PR, can you elaborate a planned algorithm for parsing with OM text then you envision (with and without assumption of _created line being last)? Maybe in pseudocode here?

// associated with. The parser cannot be used for such verification, so it is expected
// that the caller knows the order of the metrics and calls this function accordingly.
func (p *OpenMetricsParser) CreatedTimestamp() *int64 {
if p.mtype == model.MetricTypeGauge ||
p.mtype == model.MetricTypeInfo ||
p.mtype == model.MetricTypeStateset ||
p.mtype == model.MetricTypeUnknown ||
p.mtype == "" {
return nil
}
var labels labels.Labels
p.Metric(&labels)
name := labels.Get(model.MetricNameLabel)
if name[len(name)-8:] == "_created" {
ct := int64(p.val)
return &ct
}
return nil
}

Expand Down
13 changes: 12 additions & 1 deletion model/textparse/openmetricsparse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ _metric_starting_with_underscore 1
testmetric{_label_starting_with_underscore="foo"} 1
testmetric{label="\"bar\""} 1
# TYPE foo counter
foo_total 17.0 1520879607.789 # {id="counter-test"} 5`
foo_total 17.0 1520879607.789 # {id="counter-test"} 5
Copy link
Copy Markdown
Member

@bwplotka bwplotka Apr 5, 2024

Choose a reason for hiding this comment

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

Let's change order of _created to explictly show (and test) how it will be handled.

foo_created 1000
foo_total{a="b"} 17.0 1520879607.789 # {id="counter-test"} 5
foo_created{a="b"} 1000`

input += "\n# HELP metric foo\x00bar"
input += "\nnull_byte_metric{a=\"abc\x00\"} 1"
Expand Down Expand Up @@ -225,6 +228,14 @@ foo_total 17.0 1520879607.789 # {id="counter-test"} 5`
lset: labels.FromStrings("__name__", "foo_total"),
t: int64p(1520879607789),
e: &exemplar.Exemplar{Labels: labels.FromStrings("id", "counter-test"), Value: 5},
ct: 1000,
}, {
m: "foo_total{a=\"b\"}",
v: 17,
lset: labels.FromStrings("__name__", "foo_total", "a", "b"),
t: int64p(1520879607789),
e: &exemplar.Exemplar{Labels: labels.FromStrings("id", "counter-test"), Value: 5},
ct: 1000,
}, {
m: "metric",
help: "foo\x00bar",
Expand Down
9 changes: 9 additions & 0 deletions model/textparse/promparse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type expectedParse struct {
unit string
comment string
e *exemplar.Exemplar
ct int64
}

func TestPromParse(t *testing.T) {
Expand Down Expand Up @@ -212,12 +213,20 @@ func checkParseResults(t *testing.T, p Parser, exp []expectedParse) {

var e exemplar.Exemplar
found := p.Exemplar(&e)
var ct *int64
if exp[i].ct != 0 {
p.Next()
ct = p.CreatedTimestamp()
}
if exp[i].e == nil {
require.False(t, found)
} else {
require.True(t, found)
testutil.RequireEqual(t, *exp[i].e, e)
}
if exp[i].ct != 0 {
require.Equal(t, exp[i].ct, *ct)
}

case EntryType:
m, typ := p.Type()
Expand Down
159 changes: 137 additions & 22 deletions scrape/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,40 +718,148 @@ scrape_configs:
// TestManagerCTZeroIngestion tests scrape manager for CT cases.
func TestManagerCTZeroIngestion(t *testing.T) {
const mName = "expected_counter"
protobufContentType := `application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited`
openMetricsTextContentType := `application/openmetrics-text; version=0.0.1`

for _, tc := range []struct {
name string
counterSample *dto.Counter
contentType string
enableCTZeroIngestion bool

expectedValues []float64
// protobufSamples and omTextSamples are mutually exclusive.
// They must be aligned with contentType.
protobufSamples *dto.Counter
omTextSamples string

expectedValues map[string][]float64
}{
{
name: "disabled with CT on counter",
counterSample: &dto.Counter{
name: "disabled with CT on counter",
contentType: protobufContentType,
protobufSamples: &dto.Counter{
Value: proto.Float64(1.0),
// Timestamp does not matter as long as it exists in this test.
CreatedTimestamp: timestamppb.Now(),
},
expectedValues: []float64{1.0},
expectedValues: map[string][]float64{
mName: {1.0},
},
},
{
name: "enabled with CT on counter",
counterSample: &dto.Counter{
name: "protobuf/enabled with CT on counter",
contentType: protobufContentType,
protobufSamples: &dto.Counter{
Value: proto.Float64(1.0),
// Timestamp does not matter as long as it exists in this test.
CreatedTimestamp: timestamppb.Now(),
},
enableCTZeroIngestion: true,
expectedValues: []float64{0.0, 1.0},
expectedValues: map[string][]float64{
mName: {0.0, 1.0},
},
},
{
name: "enabled without CT on counter",
counterSample: &dto.Counter{
name: "protobuf/enabled without CT on counter",
contentType: protobufContentType,
protobufSamples: &dto.Counter{
Value: proto.Float64(1.0),
},
enableCTZeroIngestion: true,
expectedValues: []float64{1.0},
expectedValues: map[string][]float64{
mName: {1.0},
},
},
{
name: "OM-Text/counter without total suffix",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`# HELP %s A counter
# TYPE %s counter
%s 1.0
%s_created 1000.0
# EOF`, mName, mName, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName: {0.0, 1.0},
},
},
{
name: "OM-Text/counter with total suffix",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`# HELP %s A counter
# TYPE %s counter
%s_total 1.0
%s_created 1000.0
# EOF`, mName, mName, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName + "_total": {0.0, 1.0},
},
},
{
name: "OM-Text/counter with total and unit suffixes",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`# HELP %s_seconds A counter
# TYPE %s_seconds counter
# UNIT %s_seconds seconds
%s_seconds_total 1.0
%s_seconds_created 1000.0
# EOF`, mName, mName, mName, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName + "_seconds_total": {0.0, 1.0},
},
},
{
name: "OM-Text/unknown type",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`%s_total 1.0
%s_total_created 1000.0
# EOF`, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName + "_total": {1.0},
mName + "_total_created": {1000.0},
},
},
{
name: "OM-Text/created line unrelated to previous metric",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`# HELP %s A counter
# TYPE %s counter
%s 1.0
unrelatedmetric_created 1000.0
# EOF`, mName, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName: {1.0},
"unrelatedmetric_created": {1000.0},
},
},
{
name: "OM-Text/created line after a gauge",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`# HELP %s A gauge
# TYPE %s gauge
%s 1.0
%s_created 1000.0
# EOF`, mName, mName, mName, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName: {1.0},
mName + "_created": {1000.0},
},
},
{
name: "OM-Text/unrelated metrics without created lines",
contentType: openMetricsTextContentType,
omTextSamples: fmt.Sprintf(`%s_total 1.0
unrelatedmetrics_total 1000.0
# EOF`, mName),
enableCTZeroIngestion: true,
expectedValues: map[string][]float64{
mName + "_total": {1.0},
"unrelatedmetrics_total": {1000.0},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -786,14 +894,17 @@ func TestManagerCTZeroIngestion(t *testing.T) {
fail := true
once.Do(func() {
fail = false
w.Header().Set("Content-Type", `application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited`)

ctrType := dto.MetricType_COUNTER
w.Write(protoMarshalDelimited(t, &dto.MetricFamily{
Name: proto.String(mName),
Type: &ctrType,
Metric: []*dto.Metric{{Counter: tc.counterSample}},
}))
w.Header().Set("Content-Type", tc.contentType)
if tc.contentType == protobufContentType {
ctrType := dto.MetricType_COUNTER
w.Write(protoMarshalDelimited(t, &dto.MetricFamily{
Name: proto.String(mName),
Type: &ctrType,
Metric: []*dto.Metric{{Counter: tc.protobufSamples}},
}))
} else {
w.Write([]byte(tc.omTextSamples))
}
})

if fail {
Expand Down Expand Up @@ -822,14 +933,18 @@ func TestManagerCTZeroIngestion(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
defer cancel()
require.NoError(t, runutil.Retry(100*time.Millisecond, ctx.Done(), func() error {
if countFloatSamples(app, mName) != len(tc.expectedValues) {
return fmt.Errorf("expected %v samples", tc.expectedValues)
for mName, expectedValues := range tc.expectedValues {
if countFloatSamples(app, mName) != len(expectedValues) {
return fmt.Errorf("expected %v samples for metric %s", expectedValues, mName)
}
}
return nil
}), "after 1 minute")
scrapeManager.Stop()

require.Equal(t, tc.expectedValues, getResultFloats(app, mName))
for mName, expectedValues := range tc.expectedValues {
require.Equal(t, expectedValues, getResultFloats(app, mName))
}
})
}
}
Expand Down
Loading