Fix parsing with spaces

[shanson7]

Fixes #1672

[Dieterbe]

My take on this:

1. avoid spaces in metric names and tags whenever possible. you probably don't gain from them, and run the risk of running into issues because it.
2. allowing spaces here may trigger issues elsewhere, but i haven't done a thorough code review.
3. almost all metrictank users that i know of use carbon input, so are not using spaces because the data is carbon data. For them, this should not create any news issues
4. our current validation already allows spaces both in names and tags.
5. We can either tighten our ship and disallow spaces, but that would be breakage for those who have the data already, or just allow - but discourage - spaces.

My take is: let's merge this PR and try to support already existing spaces. keep everything else as is.
@replay your thoughts?

[Dieterbe]

Wait, when you have a target like sum(foo ) does this mean it'll extract foo as the metric name?
that seems problematic.

[shanson7]

My take on this:

1. avoid spaces in metric names and tags whenever possible. you probably don't gain from them, and run the risk of running into issues because it.

Yeah, we generally do, but we recently allowed them for particular uses and caught this case.

2. allowing spaces here may trigger issues elsewhere, but i haven't done a thorough code review.

We have been running this on prod for about 1.5 months and have been good. I know that's not exhaustive, but worth mentioning.

3. almost all metrictank users that i know of use carbon input, so are not using spaces because the data is carbon data. For them, this should not create any news issues

We don't use carbon input :)

4. our current validation already allows spaces both in names and tags.

Yes, and we adhered to the characters determined from the resolution of #1216

5. We can either tighten our ship and disallow spaces, but that would be breakage for those who have the data already, or just allow - but discourage - spaces.

Disallowing spaces would cause us a bit of an issue...

My take is: let's merge this PR and try to support already existing spaces. keep everything else as is.

Please :)

[shanson7]

Wait, when you have a target like sum(foo ) does this mean it'll extract foo as the metric name?
that seems problematic.

Hmm, That's what happens with graphite already when I use alias('foo ') to force the name, graphite successfully grabs the space character.

[Dieterbe]

i'm talking about expr.Parse which calls parseName.

func TestParse(t *testing.T) {
	e, leftover, err := Parse("sum(foo   )")
	spew.Dump(e)
	fmt.Printf("leftover %q, err %v", leftover, err)
}

=== RUN   TestParse
(*expr.expr)(0xc000286840)({
 etype: (expr.exprType) etFunc,
 float: (float64) 0,
 int: (int64) 0,
 str: (string) (len=3) "sum",
 bool: (bool) false,
 args: ([]*expr.expr) (len=1 cap=1) {
  (*expr.expr)(0xc0002868a0)({
   etype: (expr.exprType) etName,
   float: (float64) 0,
   int: (int64) 0,
   str: (string) (len=6) "foo   ",
   bool: (bool) false,
   args: ([]*expr.expr) <nil>,
   namedArgs: (map[string]*expr.expr) <nil>,
   argsStr: (string) ""
  })
 },
 namedArgs: (map[string]*expr.expr) <nil>,
 argsStr: (string) (len=6) "foo   "
})
leftover "", err <nil>--- PASS: TestParse (0.00s)
PASS
ok  	github.com/grafana/metrictank/expr	0.005s

How should Parse("alias (sum(foo ), 'myalias ' " ) parse? I think al whitespace should be ignored except for the alias 'myalias ', but i have not confirmed how graphite handles this

[replay]

I think we need to separate the questions whether spaces should be allowed in tags&values and whether spaces should be allowed in metric names.
Afaict in the graphite metric name parsing, it does not permit spaces in metric names at all, in tags they're fine. This makes sense because as @Dieterbe pointed out, how could the query parsing know whether sum(foo ) is talking about a metric "foo " or "foo". On the other hand, with tags and values we don't have this problem.

[shanson7]

Ok, it might be worth pointing out that this issue is coming from a tag value, not a metric name. If you look back at #1672 we get this because the parsing of name;tag1=value with space breaks.

[Dieterbe]

Yes but my point is that due how the code is layed out, if we apply your patch, it will affect the expression parser, which seems unintentional.

[shanson7]

Just to ping this issue, is the desired change to disallow spaces in names and modify the parser to only allow them in tags/tag values?

[replay]

Just to ping this issue, is the desired change to disallow spaces in names and modify the parser to only allow them in tags/tag values?

yes, i think that would be better

[replay]

This PR has been replaced another PR which is already merged #1848
So I think we can close this, what do you think @shanson7

[shanson7]

Yep, sounds good to me.