[processor/k8sattributes] Deprecate and remove FieldExtractConfig.Regex config option

[dmitryax]

The configuration interface of the k8sattributes receiver is over-complicated. This is an attempt to simplify it a bit.

FieldExtractConfig have KeyRegex and Regex options, which is pretty confusing. The Regex config option doesn't seem to be very important. I can be easily replaced with a transform processor with replace_pattern and replace_match functions. Therefore, I suggest:

1. Deprecate FieldExtractConfig.Regex option with instructions on how to replace it with the transform processor
2. Delete the option after 5 releases.

[bryan-aguilar]

Doc for config options.

This seems to place a requirement for a collector build to include the transform processor to match this behavior. Is introducing the requirement of a separate processor the best choice for all users? Or should the k8sattr processor be able to do this in isolation?

I think this could turn into a more meta question for the collector community. How do we decide that a dependency on another component is the correct choice?

For example for the tail sampling processor, there's a soft (hard?) dependency on the group by trace processor. I don't even know if the group by tail sampling processor example is correct. That relationship may just be a "best practice" per say. The tail sampling processor has it's own decision_wait configuration.

[dmitryax]

This one is a pretty straightforward separation of concerns. Parsing of an attribute value has nothing to do with k8s. So it should not be part of this processor. It was added a long time ago as extra functionality before we had any option to parse attribute values. Now we have the transform processor, which includes this particular feature. So we are not putting any dependency on another component, we are just removing functionality that doesn't belong to k8s attributes processor. Please let me know if it makes sense.

[dmitryax]

It may become less performant after this change, but I believe it's more important to keep a particular purpose for each component and avoid bloating and overcomplicating them with features that don't fit them. Happy to hear other opinions from @open-telemetry/collector-contrib-approvers

[shivanshuraj1333]

This one is a pretty straightforward separation of concerns. Parsing of an attribute value has nothing to do with k8s. So it should not be part of this processor. It was added a long time ago as extra functionality before we had any option to parse attribute values.

I agree.
wrt to performance concerns I'm not sure how huge the impact would be, in production users must be using transform processor and it does make sense to deprecate FieldExtractConfig.KeyRegex in lieu of transform processor.

[shivanshuraj1333]

if the proposal gets accepted, can pick up the work

[TylerHelmuth]

It may become less performant after this change, but I believe it's more important to keep a particular purpose for each component and avoid bloating and overcomplicating them with features that don't fit them.

I agree. Holding to separation of concerns is a good idea. As for performance, we can benchmark it, but the transformprocessor is quite performant.

We should not remove this feature from the processor, though, until the equivalent feature exists in the transformprocessor. It cannot currently extract new attributes using regex, we need to add the functionality via a new OTTL function.

[dmitryax]

@TylerHelmuth replace_pattern should work already, I believe. Let me know if I miss something

k8sattributes:
  extract:
    annotations:
      - tag_name: git.sha
        key: kubernetes.io/change-cause
        regex: GIT_SHA=(?P<value>\w+)

should be replaceable with

k8sattributes:
  extract:
    annotations:
      - tag_name: git.sha
        key: kubernetes.io/change-cause
transform:
  metric_statements:
    - context: resource
      statements: 
        - delete(attributes["git.sha"]) where attributes["git.sha"] != nil and not IsMatch(attributes["git.sha"], "GIT_SHA=\w+")
        - replace_pattern(attributes["git.sha"], "GIT_SHA=(\w+)", "$1") where attributes["git.sha"] != nil

But if we had extract func, it'd be cleaner:

k8sattributes:
  extract:
    annotations:
      - tag_name: k8s.change_cause
        key: kubernetes.io/change-cause
transform:
  metric_statements:
    - context: resource
      statements: 
        - extract(attributes, attributes["k8s.change_cause"], "GIT_SHA=(?P<git.sha>\w+)") where attributes["k8s.change_cause"] != nil
        - delete(attributes["k8s.change_cause"]) where attributes["k8s.change_cause"] != nil

Is this what you're thinking of?

[TylerHelmuth]

@dmitryax I forgot replace_pattern could use $1. You can remove a couple extra nil checks:

k8sattributes:
  extract:
    annotations:
      - tag_name: git.sha
        key: kubernetes.io/change-cause
transform:
  metric_statements:
    - context: resource
      statements:
        - delete(attributes["git.sha"]) where not IsMatch(attributes["git.sha"], "GIT_SHA=\w+")
        - replace_pattern(attributes["git.sha"], "GIT_SHA=(\w+)", "$1")

I see why it is so convenient to do the extraction within the k8sattributesprocessor - if you leave it for the transformprocessor then you have to explicitly check whether or not the attribute should be deleted.

If we add an extract function we could add an optional param to help with this:

k8sattributes:
  extract:
    annotations:
      - tag_name: k8s.change_cause
        key: kubernetes.io/change-cause
transform:
  metric_statements:
    - context: resource
      statements: 
        - extract(attributes, attributes["k8s.change_cause"], "GIT_SHA=(?P<git.sha>\w+)", deleteOriginal=true)