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

Fix error messaging from prometheus. #1029

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

vamsi-amazon
Copy link
Member

@vamsi-amazon vamsi-amazon commented Nov 4, 2022

Signed-off-by: vamsi-amazon reddyvam@amazon.com

Description

  • Improved messaging from prometheus response.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov-commenter
Copy link

Codecov Report

Merging #1029 (f568f81) into 2.4 (3168ed5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                2.4    #1029   +/-   ##
=========================================
  Coverage     98.26%   98.26%           
- Complexity     3326     3329    +3     
=========================================
  Files           324      324           
  Lines          8399     8403    +4     
  Branches        553      554    +1     
=========================================
+ Hits           8253     8257    +4     
  Misses          142      142           
  Partials          4        4           
Flag Coverage Δ
sql-engine 98.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...us/storage/querybuilder/StepParameterResolver.java 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vamsi-amazon vamsi-amazon force-pushed the span-fix branch 2 times, most recently from 537534b to 0f8ed02 Compare November 4, 2022 04:28
@vamsi-amazon vamsi-amazon marked this pull request as ready for review November 4, 2022 04:33
@vamsi-amazon vamsi-amazon requested a review from a team as a code owner November 4, 2022 04:33
@dai-chen
Copy link
Collaborator

dai-chen commented Nov 4, 2022

Have you confirmed if 30 and 90 days' windowing is equivalent to month and quarter in Prometheus? It seems not in our span implementation. For example, span by 1 month is expected to window the result month by month exactly. However, span by 30 days may give you window start time Jan 3 or Feb 4, because month is not fixed window size like second/minute/hour. If it's the same in Prometheus, rather than converting them, we should not push down in this case.

@vamsi-amazon
Copy link
Member Author

Prometheus doesn't support month and quarter

@vamsi-amazon
Copy link
Member Author

Are you suggesting to not push down in case month or quarter provided

Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
@vamsi-amazon vamsi-amazon changed the title Span Fix for month and quarter which prometheus doesn't support Fix error messaging from prometheus. Nov 4, 2022
@dai-chen
Copy link
Collaborator

dai-chen commented Nov 4, 2022

Are you suggesting to not push down in case month or quarter provided

Yes, either do it in post-processing or throw unsupported exception. Otherwise it would be confusing user.

Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

The response body is just what Prometheus returns to us with error details, right? Just want to double check there is no security concern here. Thanks!

@vamsi-amazon vamsi-amazon marked this pull request as ready for review November 4, 2022 17:24
@vamsi-amazon
Copy link
Member Author

The response body is just what Prometheus returns to us with error details, right? Just want to double check there is no security concern here. Thanks!

Yes.

@vamsi-amazon vamsi-amazon merged commit 4a9cef3 into opensearch-project:2.4 Nov 4, 2022
@dai-chen dai-chen added bug Something isn't working backport 2.x labels Nov 4, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 4, 2022
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit 4a9cef3)
joshuali925 pushed a commit that referenced this pull request Nov 7, 2022
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit 4a9cef3)

Co-authored-by: vamsi-amazon <reddyvam@amazon.com>
dai-chen added a commit that referenced this pull request Nov 9, 2022
* Fix `FLOAT` -> `DOUBLE` cast. (#1025)

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

* Fix error messaging from prometheus. (#1029) (#1037)

Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit 4a9cef3)

Co-authored-by: vamsi-amazon <reddyvam@amazon.com>

* Add `query` function as alternate syntax to `query_string` function (#1010)

This maintains backwards compatibility with the v1 engine.

* Update DATE and TIME functions to parse string input as datetime (#991)

Add option to accept datetime like string in both TIME and DATE (eg. accept "1999-01-02 12:12:12" for both TIME and DATE.

Strict check on date for testing for valid dates (eg. Don't accept Feb 30th as a valid date) and throws a SemanticCheckException.

Co-authored-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

* back quote fix (#1041) (#1050)

Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit d3bb902)

Co-authored-by: vamsi-amazon <reddyvam@amazon.com>

* Catalog to Datasource changes (#1027) (#1049)

Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit 3e30379)

* Bump jackson to 2.14.0 (#1058)

Signed-off-by: Joshua Li <joshuali925@gmail.com>
(cherry picked from commit 5a1adb2)

* Add valueOf() to Expression (#1055)

Signed-off-by: Joshua Li <joshuali925@gmail.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>
Signed-off-by: Joshua Li <joshuali925@gmail.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Co-authored-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: vamsi-amazon <reddyvam@amazon.com>
Co-authored-by: GabeFernandez310 <64448015+GabeFernandez310@users.noreply.github.com>
Co-authored-by: MitchellGale-BitQuill <104795536+MitchellGale-BitQuill@users.noreply.github.com>
Co-authored-by: Joshua Li <joshuali925@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants