Add log type include filter - #714
Conversation
9fcc372 to
bccd92c
Compare
|
@chombium I left a bunch of replies for points that are not yet clear to me. All the rest look good. Thanks for the thorough review! You found a bunch of nice things. |
jorbaum
left a comment
There was a problem hiding this comment.
Worked in some comments.
jorbaum
left a comment
There was a problem hiding this comment.
Worked in most comments. Still 2 left to clarify.
|
Feel free to take another look at it. |
108740d to
ef16bed
Compare
|
Thanks for the review. I replied to a bunch of comments for some clarifying remarks. Also a quick discussion about uppercase/lowercase config values would help me. |
jorbaum
left a comment
There was a problem hiding this comment.
Addressed hopefully all comments.
I got a bunch of things to clarify. Biggest question that is left is likely if we should recommend users uppercase or lowercase values.
ef16bed to
ecab32a
Compare
| } | ||
| }) | ||
|
|
||
| It("filters out the log with missing source_type when include filter is configured", func() { |
There was a problem hiding this comment.
The filter works based on the values of the source_type tag and here the code defines the behavior of what should happen.
Is this behavior documented somewhere?
There was a problem hiding this comment.
No it is not. The best place for that is likely https://docs.cloudfoundry.org/devguide/services/log-management.html ?
There was a problem hiding this comment.
Yes, that's the place. Please open a PR. Btw, why is the description of the parameters already in the docs?
There was a problem hiding this comment.
Yes, that's the place. Please open a PR.
Opened up a new PR at cloudfoundry/docs-dev-guide#584
Btw, why is the description of the parameters already in the docs?
The previous PR has been merged a bit sooner than expected.
| } | ||
| }) | ||
|
|
||
| It("filters out the log with missing source_type when include filter is configured", func() { |
There was a problem hiding this comment.
Yes, that's the place. Please open a PR.
Opened up a new PR at cloudfoundry/docs-dev-guide#584
Btw, why is the description of the parameters already in the docs?
The previous PR has been merged a bit sooner than expected.
|
I created a new branch that I rebased on #633 just in case we need it. Should also be ready for review. |
|
Hey @maecval, This change will be merged most probably next week as it needs a bit more testing. Sorry for keeping you waiting :-/ |
|
@maecval sorry to hear that. There is also a documentation draft PR that reflects the most up to date changes of this branch. |
I noticed that as my changes depend on `RawQuery` not being empty, but actually they were.
Also rename remaining instances of log type to source type
a312cd4 to
86b5879
Compare
|
Rebased on the newest changes also updated PR description. |
|
Hi @jorbaum, I did few tests with the new filter and I have that when the Then I did few tests in which I wanted to see if I get everything: container metrics and app logs except the APP logs I did get everything, including the APP logs. I guess if a value of drain-data is set, the include and exclude parameters have no effect. Please check how the parameters work along with the other Syslog Drain parameters. |
|
You are correct. Filtering was only configurable when
I also added and successfully ran a new acceptance test for drain-data=all |
chombium
left a comment
There was a problem hiding this comment.
Hi @jorbaum,
Everything looks good a part from one thing:
Please exchange include-log-source-types and exclude-log-source-types with include-log-types and exclude-log-types as we don't have types of log sources, but log types. This way we will keep the current narative as in the CF Docs Log types and their messages page.
Description
Addresses #711 :
?include-log-source-types=and?exclude-log-source-types(calling it log source type instead of log type as log type is too generic and is already used in this code base)String parsing should be efficient enough dues to:
strings.IndexBytemight take some time, but it is used only with a single char/andstrings.IndexByte()should be more efficient thanstrings.Index()logTypePrefixesrather than in each log line, which should be more efficientRelated PRs:
Disclaimer: Claude helped me in crafting this code.
Type of change
Testing performed?
Checklist:
mainbranch, or relevant version branch