[processor/k8sattributesprocessor] support key_regex match full length value #13381
Conversation
|
|
erenming
changed the title
|
Also please add a changelog entry marked as |
| @@ -1290,3 +1301,11 @@ func newTestClientWithRulesAndFilters(t *testing.T, e ExtractionRules, f Filters | |||
| func newTestClient(t *testing.T) (*WatchClient, *observer.ObservedLogs) { | |||
| return newTestClientWithRulesAndFilters(t, ExtractionRules{}, Filters{}) | |||
| } | |||
|
|
|||
| func regexpMustCompile(pattern string) *regexp.Regexp { | |||
There was a problem hiding this comment.
It's a bit inconsistent that we use this helper in this file, but hardcode the pattern in processor/k8sattributesprocessor/options_test.go. Also we need to expose RegexpCompile from this module just because of this helper.
I'd recommend removing all the helpers and hardcode ^(?:...)$ pattern when needed including processor/k8sattributesprocessor/options.go. I believe it'll be less complicated. We don't need to reuse code just for tests
There was a problem hiding this comment.
In fact, i think, the testcase in processor/k8sattributesprocessor/options_test.go is used to test the function withExtractAnnotations, so it shouldn't relay on the RegexpCompile fucntion (and so, no need to expose regexpMustCompile).
But the testcase in processor/k8sattributesprocessor/internal/kube/client_test.go is used to test the parameter KeyRegex, so it could use the helper regexpMustCompile.
This is my opinion, please correct me if i was wrong
There was a problem hiding this comment.
The purpose of the tests is different but neither of them test RegexpCompile function. Test cases that you changed in processor/k8sattributesprocessor/internal/kube/client_test.go just utilize the function, don't test it. In order to test it, you need to write or update test cases covering extractFieldRules function.
There was a problem hiding this comment.
I see, RegexpCompile is a bit useless indeed
There was a problem hiding this comment.
Fixed, but i have a problem.
Does Parameter Regex need to use full length matches? Now it's inconsistent with KeyRegex
There was a problem hiding this comment.
I'm not sure. Regex is intended to extract new fields so it doesn't need to be that strict. But I understand your concern. I would lean towards not touching Regex at this point
|
@dmitryax Hi, is there anything still need to soved? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description:
support key_regex match full length of value
Link to tracking Issue:
#9716
Testing:
test for failed match when ecounter paritial regex pattern
Documentation: