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

add function to retreive index's section memory size and use this to get symbol table size #272

Closed
wants to merge 4 commits into from

Conversation

sipian
Copy link

@sipian sipian commented Jan 31, 2018

#244
I have added a function in index/index.go to get a section's size.
Acc to documentation of index.md & this comment, a sections 1st 4 bytes holds the big endian encoded content length .

For symbol table size I retrieve these 4 bytes from (r.toc.symbols offsets)

Copy link
Collaborator

@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.

Thanks for getting started here! Now that we have the function to retrieve the data, we need to expose it as a prometheus metric.

Take this as an example: https://github.com/prometheus/tsdb/blob/master/db.go#L131-L138

We similarly need a metric which adds up all the symbol tables from all the blocks.

index/index.go Outdated
// getSectionSize returns the first 4 bytes in a section starting at offset off
// to get the content length bytes
func (r *Reader) getSectionSize(off int) int {
if off == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this check.

index/index.go Outdated
}
b := r.b.Range(off, off+4)
var l int
if l <= 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Umm, l has just been declared hence will always be 0.

index/index.go Outdated
return l
}

// getSymbolTableSize returns the bytes taken by the symbol table of Reader object
Copy link
Collaborator

Choose a reason for hiding this comment

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

end comments with .

@sipian
Copy link
Author

sipian commented Feb 9, 2018

@fabxc @brian-brazil

I have facing some difficulty in exposing symbol-table len to IndexReader interface.
The way I am doing is in index.go I've added a function GetSymbolTableSize() in struct Reader.
I need to call this function from each Block in db.block. Hence, I get IndexReader for each block(using Index()) and have added GetSymbolTableSize() in IndexReader interface in block.go.
But then we need to add implementation of GetSymbolTableSize() in head.go & querier_test.go() also.
Could you suggest a way to expose GetSymbolTableSize() to IndexReader interface?

@sipian
Copy link
Author

sipian commented Feb 13, 2018

@fabxc @brian-brazil
With the latest commit, on compiling the error is GetSymbolTableSize is not defined for IndexReader interface in head.go & querier_test.go.
To resolve this I want to know the way to expose GetSymbolTableSize function

@@ -84,6 +84,9 @@ type IndexReader interface {
// LabelIndices returns a list of string tuples for which a label value index exists.
LabelIndices() ([][]string, error)

// GetSymbolTableSize returns the size occupied by the symbol table of Reader object.
GetSymbolTableSize() uint32

Copy link
Contributor

Choose a reason for hiding this comment

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

This has nothing to do with functionality of an IndexReader and makes strong assumptions about how it is implemented. However we want to expose it, this interface isn't it.

Copy link
Author

Choose a reason for hiding this comment

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

I had the same question in this #297 PR.
There I have added a new interface IndexExtra for exposing LabelName() function.
Does this way work or is there any other way to expose functions from index.go ?

@codesome
Copy link
Contributor

codesome commented Sep 8, 2018

@sipian, do you plan to work on this?

@sipian
Copy link
Author

sipian commented Sep 8, 2018

@codesome
You can go ahead

@krasi-georgiev
Copy link
Contributor

Superseded by #375

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.

5 participants