-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
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
6798994
to
f9a4c57
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 15fcb34
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need much more tests here, for error cases, etc.
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.
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)
No description provided.