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

Jump to bottom

DO NOT MERGE: Add WIP exemplar storage #5

Closed

cstyan wants to merge 21 commits into grafana:release-2.17.1-grafana from cstyan:exemplar-grafana

cstyan commented Apr 15, 2020

Want to put the WIP exemplar storage into one of our environments so people can play around with it.

Note: at the moment this is just my branch for the upstream PR plus the latest commit to add relabelling.

cstyan requested review from gotjosh and rfratto

April 15, 2020 21:07

Member

rfratto commented Apr 16, 2020

I'm a little confused on what the next steps for this feature are. The PR here is to merge into the master branch, but AFAIK nothing is using that (not grafana/agent, anyway). Could you help me understand what the goals are here?

Member

rfratto commented Apr 16, 2020

Ah, sorry, didn't read the PR descrpition 🙃 If the point is for people to play around with it, does it really need to go into master?

cstyan changed the base branch from master to release-2.17.1-grafana

April 16, 2020 16:49

Author

cstyan commented Apr 16, 2020

@rfratto no I meant to open the PR against our 2.17.1 branch but the github UI was being janky.

rfratto approved these changes

View reviewed changes

Member

rfratto left a comment

LGTM for playing around with a WIP, minus the conflicts that need to be fixed.

If we're adding this in the agent, we'll need a follow up PR with the vendor update to add the exemplar storage into the agent's instances.

Member

rfratto commented Apr 16, 2020 •

edited

We haven't discussed this yet as part of our downstream repo strategy, but dealing with conflicts caused by other feature branches seems tricky. If you rebase, then the upstream PR will include all our other features.

I think the "right" way to do this is manually merging at the command line and fixing conflicts at merge time? It's a pain but that's the only thing I can think of.

Author

cstyan commented Apr 16, 2020

@rfratto pretty easy to fix, I just needed to rebase to our 2.17.1 branch instead of upstream 2.17.1 I didn't expect there to be merge conflicts so I didn't choose that as the base branch initially.

I don't have plans to put this into the agent in the near future, just deploy to a single instance in one environment for now.

gotjosh commented Apr 16, 2020

If that's the case then @cstyan, I don't see a reason to include it in the release-x branch. You should be able to just run it directly from your branch right?

gotjosh and others added 20 commits

April 16, 2020 12:41


          Introduce a metadata watcher

a381255

Similarly to the WAL watcher, its purpose is to observe the scrape manager and pull metadata. Then, send it to a remote storage.

Signed-off-by: gotjosh <josue@grafana.com>


          Ensure we set the scraper in the remote storage

cdd43ea


          Address previous review comments

052b309

Signed-off-by: gotjosh <josue@grafana.com>


          MVP exemplar storage separate from Appender interface.

1d0e76a

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Address some review comments + start external query flow.

fcf7b41

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Remove exemplar code from tsdb querier.

f54de42

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          ExtractSelectors from a query string.

06b8570


          use testutil instead of testify for extract selector test

13200de

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Update the exemplars query handler to handle the fact that the

c33cc43

ExtractSelectors function returns an array of arrays. Include a test.

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Add exemplar storage to scraper.

092ae05

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Pass ExemplarAppendable around, same as regular Appendable, instead of

67e6e8e

ExemplarAppender.

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Track index of previously added exemplar in exemplarList to avoid out of

53a0c96

bounds panic.

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          map exemplars in memory by labels.Labels.String() instead of Hash()

Signed-off-by: Callum Styan <callumstyan@gmail.com>

sdf

96fd658


          Make the exemplars per series a command line configurable amount.

7e644d5

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Add licesnse to tsdb/exemplar files

7d208b9

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          fix failing test and return an error properly if we can't create a

bc1f411

querier

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          WIP implementation of single fixed size buffer for all exemplars.

109817d

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Fix some things after rebasing on master

cf504d1

Signed-off-by: Callum Styan <callumstyan@gmail.com>


          Add relabelling to exemplars.

e6d19b2

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Author

cstyan commented Apr 16, 2020

@gotjosh I guess so. Could just go back to basing off 2.17.1 and run that.

Without really thinking I just thought this had to be merged into something :D

cstyan changed the title ~~Add WIP exemplar storage~~ DO NOT MERGE: Add WIP exemplar storage


          Don't panic if there's no exemplar config.

497114b

Signed-off-by: Callum Styan <callumstyan@gmail.com>

bboreham commented Feb 1, 2022

Should this be closed, as exemplars were merged upstream?

cstyan closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment