Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

Make a DB aware of the first timestamp stored in it #134

Merged
merged 1 commit into from Sep 25, 2017

Conversation

Thib17
Copy link
Contributor

@Thib17 Thib17 commented Aug 30, 2017

This is needed to fix prometheus/prometheus#3041

@mdlayher
Copy link
Contributor

Seems like you'd probably want to acquire a read-lock on mtx since it guards blocks? Also, would it make sense to return a time.Time instead of a UNIX timestamp disguised as an int64?

@fabxc
Copy link
Contributor

fabxc commented Aug 30, 2017 via email

@Thib17
Copy link
Contributor Author

Thib17 commented Aug 31, 2017

Thanks @mdlayher and @fabxc for your feedbacks. I added a lock on mtx.

@Thib17 Thib17 changed the title [WIP] Make a DB aware of the first timestamp stored in it Make a DB aware of the first timestamp stored in it Sep 4, 2017
db.go Outdated

indexr := b.Index()
joblabel := "job"
tpls, err := indexr.LabelValues(joblabel)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's leaking Prometheus semantics into TSDB, which we generally want to avoid.
I think it's somewhat questionable how well the overall query has something to do with TSDB.

It's only accessing public interfaces so it doesn't have to be in here.
Anything speaking against implementing this in prometheus/storage/tsdb?

Copy link

Choose a reason for hiding this comment

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

Could work.

Would you see another way to get the first time without having to do this hack with a label ? Maybe iterating on the chunks directly ? (I'm not super familiar with the interfaces yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabxc It doesn't only access public interfacess. It need access to blocks list which are not public yet (and I thought it was private on purpose)
@iksaif I didn't find an other way. Especially I couldn't access to a chunks list i could iterate on it.

I only see two possible solutions :
(1) make blocks public adding a BD.Blocks() func (and move the logic to prometheus/storage/tsdb)
(2) iterate on existing labels (using block.Index().LabelIndices()) instead of using "job" labels. It means keeping the same logic without prometheus specific labelname.
What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making certain methods public if there's a use case for it is perfectly valid. The exported interfaces so far are largely on a best-guess basis :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gouthamve
Copy link
Collaborator

Hmm, anything speaking against just returning the MinTime in the oldest block?

@Thib17
Copy link
Contributor Author

Thib17 commented Sep 21, 2017

@gouthamve the MinTime in the oldest block seems to be not very accurate (could be several hours from the MinTime of the first chunk AFAIK)
And adding a public method to access blocks list enable you to just select db.Blocks()[0].MinTime if you want.

This enable computing the first timestamp in the DB
only accessing public interfaces.

Signed-off-by: Thibault Chataigner <t.chataigner@criteo.com>
@gouthamve
Copy link
Collaborator

gouthamve commented Sep 25, 2017

@Thib17 Sorry for the late reply, yes, but it is not possible that there exist a sample in remote-storage that doesn't exist locally.

For example if MinT of DB = 10 and MinT of first chunk is 15, then tsdb has seen no sample between 10 and 15. Which means the remote store won't have it. If we ever delete a block for retention at t=X, we delete all samples before X and the new MinT for DB will be X.

This doesn't hold in the case of a manual series delete, but I don't think we need to optimise for that.

PS: I have nothing against a public method Blocks(), just saying that MinT is a good heuristic.

@iksaif
Copy link

iksaif commented Sep 25, 2017

@gouthamve , see prometheus/prometheus#10 (comment)

See the remote storage as something that's really durable, and local disk as something that can easily fail and get lost. This is even more true when you start running prometheus on marathon/mesos/kube/borg.

@gouthamve
Copy link
Collaborator

Yes, I agree to that.

Hmm, it will be very rare that data is corrupted in just the last chunk of a particular series. In that case, this makes sense, but in the general case, I think just looking at the MinT of the last block would work.

I am just saying instead of trying to lookup the MinTime of each series, it just makes sense to look at the db.Blocks()[0].MinTime as the mintime for each series.

In case there is corruption and you lost a block, db.Blocks()[0].MinTime would still give you the last valid block. But now with the exposure of Blocks() method, I think this is an implementation detail on Prometheus' side.

@Thib17
Copy link
Contributor Author

Thib17 commented Sep 25, 2017

@gouthamve I think we agree and move the discussion on Prometheus' side : prometheus/prometheus/pull/3129

@fabxc Is there anything missing before merging this PR ?

@gouthamve gouthamve merged commit 87c01dd into prometheus-junkyard:master Sep 25, 2017
@gouthamve
Copy link
Collaborator

Fabian is on a vacation now but this PR looks good (: Looking forward to see what comes up on prometheus/prometheus#3129

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.

Remote Storage reads based on time range in local storage or not
5 participants