-
Notifications
You must be signed in to change notification settings - Fork 1
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 tag handling in nested functions #76
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the problem is inside CopyName
, it should set the Tags correctly. Is there anywhere CopyName
is used and we don't want to copy the Tags?
Also side note: minimizing the change to CopyName
will also make upstream merges easier
I am on board with this, I just was trying to figure out if there is any situation were we wouldn't want to copy the tags. |
This comment has been minimized.
This comment has been minimized.
expr/types/types.go
Outdated
tags := make(map[string]string) | ||
for k, v := range r.Tags { | ||
tags[k] = v | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to add/overwrite the "name"
tag after that.
Actually, I think the whole function body could be replaced by this, to delete the duplicate code:
func (r *MetricData) CopyName(name string) *MetricData {
res := r.CopyLink()
if name == "" {
res.Name = name
res.Tags["name"] = name
}
return res
}
That would also mean we can delete all the r.Tags["name"] = name
after calling CopyName
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sounds like a good plan! I will make that change
This comment has been minimized.
This comment has been minimized.
b407f71
to
4c3b76a
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -34,7 +34,6 @@ func (f *aliasByMetric) Do(ctx context.Context, e parser.Expr, from, until int64 | |||
part := strings.Split(metric, ".") | |||
name := part[len(part)-1] | |||
ret := a.CopyName(name) | |||
ret.PathExpression = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this deletion intended?
b456026
to
c106ba9
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
This PR fixes an issue in which tags were not being properly set for the results of the outer function when Graphite web functions are nested. Previously, for certain functions, if there is another function is an argument that returns results, the tags that had been assigned to the resulting series are not included in the final result of the query. This is because some functions do not copy over tags. Some examples of such functions include aliasByNode, alias, and aggregateLine.
Example:
aliasByNode(absolute(testMetric),1))
The results of this query should now return the "name" tag and the "absolute" tag, instead of just the "name" tag.