Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Tagged delete #1902

Merged
merged 6 commits into from
Nov 20, 2020
Merged

Tagged delete #1902

merged 6 commits into from
Nov 20, 2020

Conversation

shanson7
Copy link
Collaborator

This PR does 2 things:

  1. Support To filter (or "older than") in tagquery filtering. This is useful for delete, where you might want to only delete series that haven't had any recent data.
  2. Support delete by tag query with an "olderThan" and dry run capability.

@replay
Copy link
Contributor

replay commented Sep 18, 2020

There's now some redundancy between the new /tags/delByQuery endpoint and the old /tags/delSeries. Unfortunately we have not much flexibility to change the /tags/delSeries endpoint, because that's part of the Graphite API, but could we maybe just extend it to also do what now /tags/delByQuery does?

The current request model for /tags/delSeries is this:

type GraphiteTagDelSeries struct {
	Paths []string `json:"path" form:"path"`
}

How about we extend it such that it looks like this:

type GraphiteTagDelSeries struct {
	Paths     []string `json:"path" form:"path"`
	Expr      []string `json:"expressions"`
	OlderThan int64    `json:"olderThan" form:"olderThan"`
	Execute   bool     `json:execute binding:"Default(false)"`
}

Then a user could simply choose whether they want to specify metrics using the metric names in .Paths or use tag query expressions (if they specify both I'd simply return an error).

Co-authored-by: Mauro Stettler <mauro.stettler@gmail.com>
@shanson7
Copy link
Collaborator Author

There's now some redundancy between the new /tags/delByQuery endpoint and the old /tags/delSeries. Unfortunately we have not much flexibility to change the /tags/delSeries endpoint, because that's part of the Graphite API, but could we maybe just extend it to also do what now /tags/delByQuery does?

I wanted to purposefully add a separate, non-graphite endpoint. I don't see the benefit of munging together a graphite compatible endpoint and a graphite incompatible endpoint. Better to have distinct specific endpoints IMO.

@Dieterbe
Copy link
Contributor

I wanted to purposefully add a separate, non-graphite endpoint. I don't see the benefit of munging together a graphite compatible endpoint and a graphite incompatible endpoint.

isn't this what we already do with many of our endpoints? eg. we added metadata format to /render and you added lastts-jso to /tags/findSeries ? this seems similar

that said, i don't care about one way or another. if that was @replay 's only last remaining remark than it seems this is ready to merge?

@shanson7
Copy link
Collaborator Author

isn't this what we already do with many of our endpoints? eg. we added metadata format to /render and you added lastts-json to /tags/findSeries ?

That's fair. I think the difference would be that there are effectively no overlapping params with the existing request.

@shanson7
Copy link
Collaborator Author

I would like to leave this as a separate endpoint. I think that it is cleaner to have more targeted endpoints than multi-functional endpoints. Also, I know that @replay has expressed concern about accidental "overdeletion" with tagged expressions.

@Dieterbe
Copy link
Contributor

@shanson7 can you just update docs/http-api.md, then this will be good to merge according to @replay

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

I think the code looks good.

As @shanson7 mentioned, my only concern is that it's easy to accidentally over-delete. Would be nice if you could add the endpoint to /docs/http-api.md and also add a warning specifically mentioning that it's easy to over delete, then I think it's fine to merge.

@Dieterbe Dieterbe merged commit fe2840f into grafana:master Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants