Add allowedLabels to remove unwanted labels from loki#1639
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1639 +/- ##
==========================================
+ Coverage 72.06% 72.16% +0.10%
==========================================
Files 165 166 +1
Lines 12718 12787 +69
==========================================
+ Hits 9165 9228 +63
- Misses 3004 3009 +5
- Partials 549 550 +1
Continue to review full report at Codecov.
|
Also fix `--log-output=loki` not pushing any logs
mstoykov
force-pushed
the
logLokiLessLabels
branch
from
6798994 to
f9a4c57
Compare
imiric
left a comment
imiric
left a comment
There was a problem hiding this comment.
Mostly LGTM, besides the parsing and tests comment.
log/loki.go
Outdated
| return nil, 0, fmt.Errorf("array item in %s need to not be empty", key) | ||
| } else if value[len(value)-1] == ']' { | ||
| value = value[:len(value)-1] | ||
| ended = true |
There was a problem hiding this comment.
This parsing seems convoluted to me... Why do you need to go over args at all? Wouldn't stripping the square brackets and doing strings.Split(value, ",") accomplish the same thing?
There was a problem hiding this comment.
I can't because I have already args = strings.Split(line, ",") early on .. so there are no more , in the values ;).
The alternative to this was to rewrite the whole code to read byte by byte and when it gets to , or [ to change what I do or something like that. This will probably be a great idea when we rewrite it to be used everywhere, but for now, it seems like too much work.
There was a problem hiding this comment.
Aha, right. Though a scanner implementation will probably be simpler than this, and shouldn't be too much work. I would prefer if we do it now instead of merging this, but I leave it up to you.
There was a problem hiding this comment.
Dude, how is that simpler? 😆 It's less hacky, I suppose, but it looks more complex than before. Why not use bufio.Scanner for this? I was picturing this would be much simpler with that approach.
There was a problem hiding this comment.
I tried using strings.Reader ( bufio.Scanner doesn't really help in any way :D). But in the end, I needed to add runes to each other to build a string to get the final keys and values out of one rune at a time (there is no "which index are we at") ... which seemed terrible and still had more issues, so I dropped it for the simplest scanning of one symbol at a time.
I don't think it's simpler but it will let us build on top of it to have "values with , inside" and {dict:values} and such.
There was a problem hiding this comment.
OK, if you think this is an improvement I'm fine with it, but I'm not sure having a custom parser for this is a good idea.
There was a problem hiding this comment.
well .. you didn't like the previous less custom ... we both don't like the strvals array syntax of array[0]=2,array[2]=1 meaning array=["2","","1"]... so I don't see another option :)
| ) | ||
|
|
||
| func TestTokenizer(t *testing.T) { | ||
| tokens, err := tokenize("loki=something,s.e=2231,s=12,12=3,a=[1,2,3],b=[1],s=c") |
There was a problem hiding this comment.
I think we need much more tests here, for error cases, etc.
There was a problem hiding this comment.
Most useful cases are in the test of the lokiHook itself. When/if we move it to a separate package for reusage - yes there should be even more test cases (for the record I did test it somewhat manually with this one testcase, just as you see the result is pretty .. big ;D)
4 participants
No description provided.