Skip to content
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

[cmd/mdatagen] Ability to filter by resource attribute values #25134

Closed
dmitryax opened this issue Aug 10, 2023 · 12 comments
Closed

[cmd/mdatagen] Ability to filter by resource attribute values #25134

dmitryax opened this issue Aug 10, 2023 · 12 comments
Labels
closed as inactive cmd/mdatagen mdatagen command enhancement New feature or request Stale

Comments

@dmitryax
Copy link
Member

dmitryax commented Aug 10, 2023

There are some scenarios when scrapers emit metrics for too many different resource attributes. We need to introduce a configuration interface generated and applied to every item in resource_attributes section that users can use to reduce the set of emitted resource attribute metrics.

We already have an option to disable resource attributes like this:

resource_attributes:
  process.command_line:
    enabled: false

We can add an option to filter resource metrics in the same configuration section.

So user can do the following:

resource_attributes:
  process.command_line:
    include:
      - "/home/dir/*"
  process.owned:
    exclude:
      - "root"

The exact filtering interface is still to be defined.

Originally suggested in #8188 (comment)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@dmitryax
Copy link
Member Author

dmitryax commented Nov 3, 2023

For the include/exclude config interface, we need to stick to the new standardized approach proposed in #25161. People are more in favor of option B, but we still have time to reconsider it.

@povilasv
Copy link
Contributor

povilasv commented Dec 12, 2023

👋 Have a very raw draft PR for this functionality, would love to get some reviews if this is the approach we would like to take.

Basically it implements this kind of interface:

string.enum.resource.attr:
    description: Resource attribute with a known set of string values.
    type: string
    enum: [one, two]
    enabled: true
    exclude:
      - strict: two
    include:
      - strict: one
      - regex: one|two
  1. I implemented small filter package for filter config and matching (https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/29769/files#diff-73dd80db8a6a9e9d17674e3d3a868caadb9b1a01cb64c24427b6f8fde4725e3bR12)
  2. In order to initialize the regexes we generate code in NewResourceAttributeBuilder (can be improved) . Example -> https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/29769/files#diff-493333c8cd004749e934daeb3a36e2be37027bf7b4c8c2896c9cea13fcb93655R47-R53

NewResourceBuilder will need to now return an error, since it needs to compile the regexes.

  1. I add the calls to includeFilter.Matches(val) in Set methods. https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/29769/files#diff-493333c8cd004749e934daeb3a36e2be37027bf7b4c8c2896c9cea13fcb93655R92-R104

We still have the open question around what do we do with types such as int, double, bool, bytes, slice and map?

ATM we generate only for string setters

cc @dmitryax

@dmitryax
Copy link
Member Author

dmitryax commented Jan 2, 2024

We still have the open question around what do we do with types such as int, double, bool, bytes, slice and map?

I think those should only accept strict filtering

Copy link
Contributor

github-actions bot commented Mar 4, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Mar 4, 2024
dmitryax added a commit to open-telemetry/opentelemetry-collector that referenced this issue Apr 12, 2024
…9660)

**Description:**
This PR allows filtering out metrics based on resource attributes. For
example:

```
        resource_attributes:                                                                      
          k8s.pod.name:
            enabled: true 
            exclude:
              #- strict: "kube-apiserver-kind-control-plane"
              - regexp: "kube-.*"
              - regexp: "coredns-.*"
              - strict: "coredns"
              - strict: "kindnet-mpb2p"
```

Would remove metrics that match regex or strict rules on resource
attributes.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector-contrib#25134

**Testing:** 
- Tested with k8scluster receiver in kind cluster.
- unit tests added
**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@dmitryax dmitryax removed the Stale label Apr 13, 2024
@andrzej-stencel
Copy link
Member

andrzej-stencel commented Apr 17, 2024

We can add an option to filter resource metrics in the resource_attributes configuration section.

So user can do the following:

resource_attributes:
  process.command_line:
    include:
      - "/home/dir/*"
  process.owned:
    exclude:
      - "root"

The exact filtering interface is still to be defined.

Originally suggested in #8188 (comment)

I have two problems with this approach:

  1. I think the syntax resource_attributes.<attr-name>.metrics_include... makes it hard to understand what it does (i.e. drop metrics whose specific resource attribute matches a condition). Is it only me having problem internalizing this?

  2. This solution does not solve the original requester's problem as described in this comment.

I believe the right solution to this is to use OTTL, as we're dealing with filtering/transforming telemetry here (metrics specifically) and OTTL was specifically designed for these scenarios.

Given the currently implemented solution in #9660 and #9960, here's the closest I could get to achieving the requester's goal from this comment:

Goal: Only emit the following process metrics:

  • Emit process metrics that have process.executable.name equal to otel-collector or collector2, but not whose command line starts with test
  • Also emit process metrics that have process.command equal to java and process.command_line equal to minecraft
receivers:
  hostmetrics:
    scrapers:
      process:
        resource_attributes:
          process.executable.name:
            metrics_include:
              - strict: otel-collector
              - strict: collector2
          process.command_line:
            metrics_exclude:
              - regexp: ^test

Note that with this syntax it's not possible to satisfy the requester's goal fully. It's not possible to also allow the "java"/"minecraft" metrics.

Here's what a configuration based on OTTL would look like (this is similar to how Filter processor would be configured):

receivers:
  hostmetrics:
    scrapers:
      process:
        metrics:
          filters:
            resource:
              - 'IsMatch(attributes["process.command_line"], "^test") or !IsMatch(attributes["process.executable.name"], "^otel-collector$|^collector2$")'
              - 'attributes["process.command"] != "java" or attributes["process.command_line"] == "minecraft"'

Actually I believe this is still hard to read, write and understand due the user needing to negate the exclusion logic to achieve an inclusion logic.

I propose to allow the user to specify either an inclusion filter or an exclusion filter (or maybe both, with exclusion applied after inclusion). Here's what it would look like:

receivers:
  hostmetrics:
    scrapers:
      process:
        metrics:
          include[_filters]:
            resource:
              - '(attributes["process.executable.name"] == "otel-collector" or attributes["process.executable.name"] == "collector2") and !IsMatch(attributes["process.command_line"], "^test")'
              - 'attributes["process.command"] == "java" and attributes["process.command_line"] == "minecraft"'

Ideally we should be able to extend this OTTL postprocessing of telemetry to other signals than metrics - logs, traces at al. But this issue is only about metrics, so I propose to put it in metrics.include|exclude[_filters]... configuration.

dmitryax added a commit to open-telemetry/opentelemetry-collector that referenced this issue Apr 22, 2024
The `include` and `exclude ` options in the resource attributes group
sound confusing. It's easy to assume that matching filters will include
or exclude resource attributes themselves while they control emitted
resource metrics.

The proposal is to change the include/exclude options to
`metrics_include`/`metrics_exclude` with detailed comments. These names
make it cleaner that matching rules limit the emitted metrics, not
resource attributes.

Updates
open-telemetry/opentelemetry-collector-contrib#25134
dmitryax added a commit that referenced this issue Apr 22, 2024
@dmitryax
Copy link
Member Author

@andrzej-stencel I understand that some complicated use cases might not be covered by this interface. And I don't think we need to aim for that. Any complicated use case can be solved by adding the filter processor.

The intent behind this issue is to provide a simple interface to control what resources are "allowed" to emit metrics. It'll be a pretty important feature for scraping receivers collecting data with many resources.

For example, in the k8scluster receiver, if I want to simply exclude all metrics from the kube-* namespaces I can easily do

receivers:
  k8scluster:
    resource_attributes:
      k8s.cluster.name:
        metrics_exclude:
          - regex: kube-.*

which seems pretty straight-forward to use compared to

receivers:
  k8scluster:
    metrics:
      include:
        resource:
          - '!IsMatch(attributes["k8s.cluster.name"], "kube-.*")'

I see two problems with the approach you suggest:

  1. Users have to type attributes as strings which is error-prone, if an attribute is renamed it's easy to miss updating the string.
  2. The metrics config section has metric names as keys. Adding include or include_filter creates confusion and restricts metrics with the same name, even if it's unlikely a component will have an include metric.

@dmitryax
Copy link
Member Author

@povilasv I'd like to hear your input here and what's your use case

@povilasv
Copy link
Contributor

I basically agree with everything you @dmitryax said.

I think OTTL option was discussed in #25161 and Sig meeting (if I recall correctly). The use case here is basically simple way to filter data, similiar to what we have in hostmetric processor but just for mdatagen receivers.

This approach we took IMO solves it and it is applied broadly. Do you want to monitor just x namespace, you can do it, just y Mongo / redis / .. instance, you can do it.

@povilasv
Copy link
Contributor

BTW I think we can close this ticket as done. Any changes in interface we should probably discuss in #25161

andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this issue May 27, 2024
The `include` and `exclude ` options in the resource attributes group
sound confusing. It's easy to assume that matching filters will include
or exclude resource attributes themselves while they control emitted
resource metrics.

The proposal is to change the include/exclude options to
`metrics_include`/`metrics_exclude` with detailed comments. These names
make it cleaner that matching rules limit the emitted metrics, not
resource attributes.

Updates
open-telemetry/opentelemetry-collector-contrib#25134
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Jun 25, 2024
Copy link
Contributor

This issue has been closed as inactive because it has been stale for 120 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed as inactive cmd/mdatagen mdatagen command enhancement New feature or request Stale
Projects
None yet
Development

No branches or pull requests

3 participants