Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

aliasSub legend name incorrect when preceeded by sumSeries function #648

Closed
Dieterbe opened this issue Jun 2, 2017 · 8 comments
Closed

Comments

@Dieterbe
Copy link
Contributor

Dieterbe commented Jun 2, 2017

can be seen when comparing topleft chart between regular MT dashboard (works fine, has no extra sumSeries) vs MT k8s cluster (has extra sumSeries):

aliasSub(perSecond(metrictank.stats.$environment.$instance.input.*.metric_invalid.counter32),'.*\.([^\.]+)\.metric_invalid.*', '\1 metric invalid')
=> 3 entries with name 'kafka-mdm invalid' (correct)

aliasSub(sumSeries(perSecond(metrictank.stats.$environment.$instance.input.*.metric_invalid.counter32)),'.*\.([^\.]+)\.metric_invalid.*', '\1 metric invalid')
=> creates a name '* in' (i don't believe graphite does this)

this bug may also affect other functions like aliasByNode.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jun 23, 2017

actually what graphite does is not correct either. it's a bit harder to reproduce because we use the kafka-mdm input everywhere and nothing else, but let's pretend the instance name is the name of the input, so that we have some variety, which lets me highlight the problem in graphite.
below i show the series that go into the sumseries/alias sub, and then we do the aliassub, but change the aliasSub argument so that it uses instance name instead of input name .

so what graphite does in this case, it'll just take the name of the last series, and use that in the aliassub.
this is confusing, because other series with different names went into the sumSeries !
1-how-graphite-does-it
what MT does is just show * which is not like graphite, but now that I think about it, more clear that different series went into the result
2-how-mt-does-it

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jun 23, 2017

let's do the same experiment, but with only 1 matching series:
here graphite and MT behave the same as before. graphite is not incorrect anymore (seems coincidental in this case) though it can be argued that its output is now a bit more useful than MT's is.
graphite :
case2-graphite
mt :
case2mt

@Dieterbe
Copy link
Contributor Author

so I think MT should keep using * when multiple series went into a sum that feeds into an aliasByNode/aliasSub function, rather then being misleading like graphite. though if only 1 series went into it, it can be clearer like graphite. (if multiple distinct series go into aliasByNode/aliasSub directly, then can each be separately accurately aliased of course, like before)
thoughts @DanCech @woodsaj

@Dieterbe
Copy link
Contributor Author

actually, since sum(metrictank.stats.*ops.*dr59.input.*.metric_invalid.counter32) (despite using wildcards, only results in 1 series) results in a target output of sumSeries(metrictank.stats.*ops.*dr59.input.*.metric_invalid.counter32) for both graphite and MT, I think it's more than fair that we stay consistent with that and also use "*" in the aliasByNode/aliasSub output if a wildcard pattern was used to feed into a sumseries/avgseries.

so I think we can just close this and be happy with the current state of things.

@woodsaj
Copy link
Member

woodsaj commented Jun 24, 2017

regardless of what is the more correct behaviour, a dashboard using Graphite should continue working when data is moved to MT.

@Dieterbe
Copy link
Contributor Author

a dashboard using Graphite should continue working when data is moved to MT.

what do you mean exactly with "keep working" ? with my proposed outcome, the only edge case I can think of is a display rule might match differently, but then again it would also match differently with graphite when a series would be phased out that is matched on. do you have anything specific against my proposition?

@woodsaj
Copy link
Member

woodsaj commented Jun 27, 2017

It is still important for MT to behave the same as graphite. If we conclude that graphite is doing something wrong and we want to correct it in MT, then we need to open an issue in graphite to have it fixed (if possible).

So open an issue in graphite, then this is good to close.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jun 28, 2017

actually it seems the latest graphite-web (i tested 1.0.1) now works like MT does, i tested both aliasByNode and aliasSub :

docker run -p 8080:8080 vladwing/docker-graphite # i also tested movingMin() to make sure it's >=1.0
~ ❯❯❯ curl -s 'http://localhost:8080/render/?target=carbon.agents.*.a*&from=-1minutes&format=json' | jsonpp | grep target
        "target": "carbon.agents.098a020e9c77-a.activeConnections",
        "target": "carbon.agents.098a020e9c77-a.avgUpdateTime",
~ ❯❯❯ curl -s 'http://localhost:8080/render/?target=sumSeries(carbon.agents.*.a*)&from=-1minutes&format=json' | jsonpp | grep target                                                         ⏎
        "target": "sumSeries(carbon.agents.*.a*)",
~ ❯❯❯ curl -s 'http://localhost:8080/render/?target=aliasByNode(sumSeries(carbon.agents.*.a*),3)&from=-1minutes&format=json' | jsonpp | grep target                                          ⏎
        "target": "a*",
~ ❯❯❯ curl -s "http://localhost:8080/render/?target=aliasSub(sumSeries(carbon.agents.*.a*),'carbon','foo')&from=-1minutes&format=json"
[{"target": "sumSeries(foo.agents.*.a*)", "datapoints": [[null, 1498650960]]}]% 

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants