Skip to content

[query] when using unless op do not remove metric name#2418

Open
gediminasgu wants to merge 2 commits into
masterfrom
gg/unless-op-missing-name-fix
Open

[query] when using unless op do not remove metric name#2418
gediminasgu wants to merge 2 commits into
masterfrom
gg/unless-op-missing-name-fix

Conversation

@gediminasgu
Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:
PromQL compatibility fix.
When using unless operator in the query it should not remove metric names from the result.

Sample query:
http_requests{group="canary"} unless http_requests{instance="0"}

The result before the fix:
{group="canary", instance="1", job="api-server"}

The result after the fix:
http_requests{group="canary", instance="1", job="api-server"}

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

Comment thread src/query/functions/binary/unless.go

# FAILING issue #35. eval instant at 50m http_requests{group="canary"} unless on(job) http_requests{instance="0"}

# FAILING issue #34. eval instant at 50m http_requests{group="canary"} unless on(job, instance) http_requests{instance="0"}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What about other #34 cases that we have? If they don't get fixed by this change, might mean that they are a different issue.

Copy link
Copy Markdown
Collaborator

@arnikola arnikola Jun 17, 2020

Choose a reason for hiding this comment

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

+1, looks like we should be able to have the same fix in or.go, also looks like #23 might be solved by the same fix in and.go

Copy link
Copy Markdown
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

Looks good, worth adding similar and and or fixes?

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 11, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.5%. Comparing base (b12af4d) to head (704afa0).
⚠️ Report is 1409 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2418   +/-   ##
======================================
  Coverage    71.5%   71.5%           
======================================
  Files        1063    1063           
  Lines       93410   93408    -2     
======================================
+ Hits        66825   66827    +2     
  Misses      22093   22093           
+ Partials     4492    4488    -4     
Flag Coverage Δ
aggregator 76.4% <ø> (+<0.1%) ⬆️
cluster 84.8% <ø> (+37.6%) ⬆️
collector 82.8% <ø> (+35.3%) ⬆️
dbnode 79.0% <ø> (-0.1%) ⬇️
m3em 74.4% <ø> (ø)
m3ninx 73.3% <ø> (+<0.1%) ⬆️
m3nsch 51.1% <ø> (+22.7%) ⬆️
metrics 17.2% <ø> (-31.5%) ⬇️
msg 75.1% <ø> (-0.2%) ⬇️
query 67.4% <ø> (-1.1%) ⬇️
x 81.8% <ø> (+0.2%) ⬆️

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


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b12af4d...704afa0. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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