Skip to content
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.

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

Prometheus: Fix creation of invalid dataframes with exemplars #75187

Merged
merged 1 commit into from Sep 21, 2023

Conversation

kylebrandt
Copy link
Contributor

@kylebrandt kylebrandt commented Sep 20, 2023

With different labels for examplars, the prom response parser would create invalid frames (different field lengths). The prom datasource would then panic processing these frames.

This adds a test case for it and addresses the issue.

Which issue(s) does this PR fix?:

Fixes #73654

Special notes for your reviewer:

It was creating something like the following:

(*data.Frame)(0xc000b3f7c0)({
 Name: (string) "",
 Fields: ([]*data.Field) (len=4 cap=4) {
  (*data.Field)(0xc000aadaa0)({
   Name: (string) (len=4) "Time",
   Labels: (data.Labels) ,
   Config: (*data.FieldConfig)(<nil>),
   vector: (*data.timeTimeVector)(0xc0002018d8)((len=2 cap=2) {
    (time.Time) 2022-06-01 12:28:36.81 +0000 UTC,
    (time.Time) 2031-12-03 17:48:36.81 +0000 UTC
   })
  }),
  (*data.Field)(0xc000aadad0)({
   Name: (string) (len=5) "Value",
   Labels: (data.Labels) (len=9) __name__=traces_spanmetrics_duration_seconds_bucket, group=mythical, instance=f1c32fe505c3, le=0.256, service=mythical-requester, source=tempo, span_kind=SPAN_KIND_CLIENT, span_name=requester, span_status=STATUS_CODE_OK,
   Config: (*data.FieldConfig)(<nil>),
   vector: (*data.float64Vector)(0xc0002018f0)((len=2 cap=2) {
    (float64) 1.134971,
    (float64) 1.134971
   })
  }),
  (*data.Field)(0xc000aadb30)({
   Name: (string) (len=7) "traceID",
   Labels: (data.Labels) ,
   Config: (*data.FieldConfig)(<nil>),
   vector: (*data.stringVector)(0xc000201908)((len=1 cap=1) {
    (string) (len=32) "46e15ae41852149068aeb26c64643c29"
   })
  }),
  (*data.Field)(0xc000aadb60)({
   Name: (string) (len=8) "ztraceID",
   Labels: (data.Labels) ,
   Config: (*data.FieldConfig)(<nil>),
   vector: (*data.stringVector)(0xc000201950)((len=1 cap=1) {
    (string) (len=32) "46e15ae41852149068aeb26c64643c29"
   })
  })
 },

Which wouldn't marshal to JSON, so if we had the test case the existing tests would have failed.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@@ -417,7 +418,6 @@ l1Fields:
timeField.Append(ts)

case "labels":
max := 0
pairs, err := readLabelsAsPairs(iter)
Copy link
Member

Choose a reason for hiding this comment

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

not really related to this PR, but one thing i just noticed while reviewing this locally is that we don't seem to cover an exemplar without labels in tests.

@kylebrandt kylebrandt merged commit b87279b into main Sep 21, 2023
29 checks passed
@kylebrandt kylebrandt deleted the promConvertExemplarFix branch September 21, 2023 11:30
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exemplars: Grafana panics if any time series has >1 unique label name
4 participants