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. |
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
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.
Description
Addresses #711 :
?include-source-types=and?exclude-source-types(calling it source type instead of log type as log type is too generic and is already used in this code base)I still need to verify this, but I think a feature flag is not necessary as I tried to make string parsing more efficient by:
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