Skip to content
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

Make "range vector" and "matrix" terminology consistent #2196

Closed
tcolgate opened this Issue Nov 17, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@tcolgate
Copy link
Contributor

tcolgate commented Nov 17, 2016

What did you do?

make an error in a query where a function expects a range vector, but you just give it an instant vector

What did you expect to see?

An error that refers to requiring a range vector

What did you see instead? Under which circumstances?

An error that refers to a "matrix"

The documentation doesn't really ever refer to Prometheus supporting a "matrix" type, but the errors do. Equally, some errors state that a "vector" has been required where a "matrix" is expected, yet the docs refer to instant vectors and range vectors (so both are vectors, by the wording in the docs).

It would be nice if the terminology was consistent between the two.

@tcolgate

This comment has been minimized.

Copy link
Contributor Author

tcolgate commented Nov 17, 2016

I can look at updating some error messages. I think replacing "vector" with "instant vector", and "matrix" with "range vector", would be OK?

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Nov 17, 2016

That touches ancient Prometheus history, so I'd say @juliusv should comment on it.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 17, 2016

Renaming as you suggest makes sense. The older terminology is a holdover from Borgmon.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Nov 17, 2016

We also call it matrix / vector in the API though: https://prometheus.io/docs/querying/api/#expression-query-result-formats

So the inconsistency there would have to remain unfortunately.

@tcolgate

This comment has been minimized.

Copy link
Contributor Author

tcolgate commented Nov 17, 2016

Initial attempt in #2197
I don't think the type names in common/model can realistically be changed, so this attempt to translate it purely for the purposes of messages reported to the users.
I could use a type, with a String() method, but that seemed like overkill
Some tests still failing, just wanted to see if this was sane.

@tcolgate

This comment has been minimized.

Copy link
Contributor Author

tcolgate commented Nov 21, 2016

#2197 was merged

@tcolgate tcolgate closed this Nov 21, 2016

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.