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.

Sign up for GitHub

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

Jump to bottom

Fix error messaging from prometheus. #1029

Merged
merged 1 commit into from
Nov 4, 2022
Merged

Fix error messaging from prometheus. #1029

merged 1 commit into from
Nov 4, 2022

Conversation

vmmusings
Copy link
Member

@vmmusings vmmusings commented Nov 4, 2022 •
edited
Loading

Signed-off-by: vamsi-amazon [email protected]

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.

@vmmusings vmmusings force-pushed the span-fix branch 2 times, most recently from 537534b to 0f8ed02 Compare November 4, 2022 04:28
@vmmusings vmmusings marked this pull request as ready for review November 4, 2022 04:33
@vmmusings vmmusings 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.

@vmmusings
Copy link
Member Author

Prometheus doesn't support month and quarter

@vmmusings
Copy link
Member Author

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

@vmmusings vmmusings marked this pull request as draft November 4, 2022 17:04
@vmmusings vmmusings 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.

dai-chen
dai-chen approved these changes Nov 4, 2022
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment •
edited by vmmusings
Loading

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!

@vmmusings vmmusings marked this pull request as ready for review November 4, 2022 17:24
@vmmusings
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.

@vmmusings vmmusings 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
@vmmusings @github-actions
Fix error messaging from prometheus. (#1029)
d833510 
Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit 4a9cef3)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 4, 2022
Merged
joshuali925 pushed a commit that referenced this pull request Nov 7, 2022
@opensearch-trigger-bot @vmmusings
Fix error messaging from prometheus. (#1029) (#1037)
a4c7cf7 
Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit 4a9cef3)

Co-authored-by: vamsi-amazon <[email protected]>
dai-chen added a commit that referenced this pull request Nov 9, 2022
@dai-chen @Yury-Fridlyand
@opensearch-trigger-bot @vmmusings @GabeFernandez310 @MitchellGale @joshuali925
Merge 2.x to Maximus feature branch (#1062)
3c6b37a 
* Fix `FLOAT` -> `DOUBLE` cast. (#1025)

Signed-off-by: Yury-Fridlyand <[email protected]>

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

Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit 4a9cef3)

Co-authored-by: vamsi-amazon <[email protected]>

* 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 <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>

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

Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit d3bb902)

Co-authored-by: vamsi-amazon <[email protected]>

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

Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit 3e30379)

* Bump jackson to 2.14.0 (#1058)

Signed-off-by: Joshua Li <[email protected]>
(cherry picked from commit 5a1adb2)

* Add valueOf() to Expression (#1055)

Signed-off-by: Joshua Li <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: Joshua Li <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: vamsi-amazon <[email protected]>
Co-authored-by: GabeFernandez310 <[email protected]>
Co-authored-by: MitchellGale-BitQuill <[email protected]>
Co-authored-by: Joshua Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sejli sejli sejli approved these changes

@dai-chen dai-chen dai-chen approved these changes

@derek-ho derek-ho derek-ho approved these changes

Assignees
No one assigned
Labels
backport 2.x bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vmmusings @codecov-commenter @dai-chen @derek-ho @sejli