Skip to content

Add /series support to TSDB storage #1830

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

Conversation

pracucci
Copy link
Contributor

What this PR does:
Currently the /series API endpoint doesn't work with the TSDB storage, due to missing implementation of MetricsForLabelMatchers(). In this PR I'm addressing this issue.

Which issue(s) this PR fixes:
No issue created.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci
Copy link
Contributor Author

@thorfour and @codesome may you add to review this PR to your backlog, please?

@pracucci pracucci force-pushed the add-metrics-for-label-matchers-to-tsdb-storage branch from e002ffc to 30f59fd Compare November 15, 2019 15:04
Copy link
Contributor

@thorfour thorfour left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise LGTM Thanks!

}

// Create a new instance of the TSDB querier
q, err := db.Querier(from.Unix()*1000, to.Unix()*1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit prefer time.Microsecond here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually even easier: from and to are model.Time (not the Go time pkg), and are already expressed in milliseconds, so we've just to cast it to int64.

What's your take?

Copy link
Contributor

Choose a reason for hiding this comment

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

that sounds good

@pracucci pracucci force-pushed the add-metrics-for-label-matchers-to-tsdb-storage branch from bd26db9 to 976e374 Compare November 26, 2019 13:34
@pracucci pracucci force-pushed the add-metrics-for-label-matchers-to-tsdb-storage branch from e1969c5 to 540648b Compare December 4, 2019 11:01
// already been added to the result
ls := seriesSet.At().Labels()
key := ls.String()
if _, ok := added[key]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be consistent with upstream, can we use this: https://github.com/prometheus/prometheus/blob/0ae4899c47fbcdab1b0bebedf213b4424f50f760/web/api/v1/api.go#L546-L550

It's an exposed function and I see it fitting perfectly here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked it. The storage.NewMergeSeriesSet() you pointed out works with the storage pkg structs, while here we work with tsdb structs (incompatible types). I checked the merge functions in tsdb pkg and looks a bit overkilled for what we need here: they merge series samples, while here we just need a unique set of labels.

I would suggest to keep this code as is.

Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM with nit

@pracucci pracucci force-pushed the add-metrics-for-label-matchers-to-tsdb-storage branch from 540648b to 726ece2 Compare December 6, 2019 08:55
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

@jtlisi jtlisi merged commit 3329a99 into cortexproject:master Dec 11, 2019
@pracucci pracucci deleted the add-metrics-for-label-matchers-to-tsdb-storage branch December 12, 2019 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants