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

vmselect: fix label_replace when mismatch #579

Merged
merged 1 commit into from Jun 23, 2020
Merged

vmselect: fix label_replace when mismatch #579

merged 1 commit into from Jun 23, 2020

Conversation

nicbaz
Copy link
Contributor

@nicbaz nicbaz commented Jun 22, 2020

As per documentation on label_replace function: "If the regular
expression doesn't match then the timeseries is returned unchanged".

Currently this behavior is not enforced, if a regexp on an existing
tag doesn't match then the tag value is copied as-is in the destination
tag. This fix first checks that the regular expression matches the
source tag before applying anything.

Given the current implementation, this fix also changes the behavior
of the MetricsQL label_transform function which does not
document this behavior at the moment.

As per documentation on `label_replace` function: "If the regular
expression doesn't match then the timeseries is returned unchanged".

Currently this behavior is not enforced, if a regexp on an existing
tag doesn't match then the tag value is copied as-is in the destination
tag. This fix first checks that the regular expression matches the
source tag before applying anything.

Given the current implementation, this fix also changes the behavior
of the **MetricsQL** `label_transform` function which does not
document this behavior at the moment.
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #579 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   68.18%   68.27%   +0.09%     
==========================================
  Files         151      151              
  Lines       21081    21083       +2     
==========================================
+ Hits        14374    14395      +21     
+ Misses       5554     5529      -25     
- Partials     1153     1159       +6     
Impacted Files Coverage Δ
app/vmselect/promql/transform.go 87.12% <100.00%> (-0.17%) ⬇️
app/vmselect/promql/aggr_incremental.go 90.72% <0.00%> (-1.38%) ⬇️
lib/storage/partition.go 71.54% <0.00%> (ø)
lib/storage/index_db.go 76.03% <0.00%> (+0.05%) ⬆️
lib/mergeset/table.go 68.29% <0.00%> (+0.55%) ⬆️
lib/storage/part.go 74.48% <0.00%> (+1.37%) ⬆️
lib/mergeset/block_stream_reader.go 62.58% <0.00%> (+11.61%) ⬆️

Continue to review full report at Codecov.

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

Copy link
Collaborator

@valyala valyala left a comment

Choose a reason for hiding this comment

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

LGTM

@valyala
Copy link
Collaborator

valyala commented Jun 23, 2020

@nicbaz , thanks for the contribution!

@valyala valyala merged commit 774f7ca into VictoriaMetrics:master Jun 23, 2020
valyala pushed a commit that referenced this pull request Jun 23, 2020
As per documentation on `label_replace` function: "If the regular
expression doesn't match then the timeseries is returned unchanged".

Currently this behavior is not enforced, if a regexp on an existing
tag doesn't match then the tag value is copied as-is in the destination
tag. This fix first checks that the regular expression matches the
source tag before applying anything.

Given the current implementation, this fix also changes the behavior
of the **MetricsQL** `label_transform` function which does not
document this behavior at the moment.
@valyala
Copy link
Collaborator

valyala commented Jun 25, 2020

FYI, the pull request has been included in v1.37.3.

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.

None yet

2 participants