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

engine: remove timestamp function so that we fallback to promql again #204

Merged

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Mar 6, 2023

Prometheus returns the timestamp of the last sample in the lookback period, we currently only return 0 because we never pass a timestamp to the point that is passed to the function. It is not clear how to fix this since we do not save the timestamp of the last sample in the lookback period currently. Notice that prometheus returns 30 even at timestamp 90000

                    Diff:
                    --- Expected
                    +++ Actual
                    @@ -2,46 +2,46 @@
                     0 @[0]
                    -30 @[30000]
                    -30 @[60000]
                    -30 @[90000]
                    -30 @[120000]
                    -30 @[150000]
                    -30 @[180000]
                    -30 @[210000]
                    -30 @[240000]
                    -30 @[270000]
                    -30 @[300000]
                    -30 @[330000]
                    -30 @[360000]
                    -30 @[390000]
                    -30 @[420000]
                    -30 @[450000]
                    -30 @[480000]
                    -30 @[510000]
                    -30 @[540000]
                    -30 @[570000]
                    -30 @[600000]
                    -30 @[630000]
                    +0 @[30000]
                    +0 @[60000]
                    +0 @[90000]
                    +0 @[120000]
                    +0 @[150000]
                    +0 @[180000]
                    +0 @[210000]
                    +0 @[240000]
                    +0 @[270000]
                    +0 @[300000]
                    +0 @[330000]
                    +0 @[360000]
                    +0 @[390000]
                    +0 @[420000]
                    +0 @[450000]
                    +0 @[480000]
                    +0 @[510000]
                    +0 @[540000]
                    +0 @[570000]
                    +0 @[600000]
                    +0 @[630000]
                     {pod="nginx-2"} =>
                     0 @[0]
                    -30 @[30000]
                    -30 @[60000]
                    -30 @[90000]
                    -30 @[120000]
                    -30 @[150000]
                    -30 @[180000]
                    -30 @[210000]
                    -30 @[240000]
                    -30 @[270000]
                    -30 @[300000]
                    -30 @[330000]
                    -30 @[360000]
                    -30 @[390000]
                    -30 @[420000]
                    -30 @[450000]
                    -30 @[480000]
                    -30 @[510000]
                    -30 @[540000]
                    -30 @[570000]
                    -30 @[600000]
                    -30 @[630000])
                    +0 @[30000]
                    +0 @[60000]
                    +0 @[90000]
                    +0 @[120000]
                    +0 @[150000]...(output trimmed)

@MichaHoffmann MichaHoffmann changed the title engine: add broken test for timestamp function engine: remove timestamp function so that we fallback to promql again Mar 6, 2023
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-timestamp-function-is-broken branch from 0e3671d to 01f49de Compare March 6, 2023 15:50
Copy link
Collaborator

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, I agree we should roll this back.

@yeya24
Copy link
Contributor

yeya24 commented Mar 6, 2023

Let's create a separate issue to support timestamp function?

@MichaHoffmann
Copy link
Contributor Author

Let's create a separate issue to support timestamp function?

That might be non trivial and require changes to StepVector to save timestamps of samples i fear. We could open an issue for it but i wonder if the function warrants the changes.

@yeya24
Copy link
Contributor

yeya24 commented Mar 6, 2023

Yes, but in the long run we have to support it natively in the engine, right? Then let's have an issue for sure and make required changes if needed.
I guess we don't want to fallback to the old engine forever.

@fpetkovski
Copy link
Collaborator

I think we could to push down the timestamp func into the selector and store the timestamp in the value for each series. Having an issue to track this makes sense since it’s a commonly used function.

@MichaHoffmann
Copy link
Contributor Author

We could push the timestamp function down into the vector selector maybe, thats where we have the information easily available and timestamp only makes sense in timestamp(X) form with X being a vector selector, right? That way we dont need to modify StepVector directly.

@fpetkovski
Copy link
Collaborator

I've opened #206 to track this.

Are you okay with merging this @yeya24?

@yeya24
Copy link
Contributor

yeya24 commented Mar 7, 2023

Thanks. Let's merge it.

@yeya24 yeya24 merged commit 9db8e21 into thanos-io:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants