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

[mdatagen] Rename include/exclude config options #9960

Merged

Conversation

dmitryax
Copy link
Member

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 dmitryax requested review from a team and TylerHelmuth April 12, 2024 22:36
@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 12, 2024
@dmitryax
Copy link
Member Author

No changelog entry is needed since this functionality is not released yet

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.75%. Comparing base (d0f15e2) to head (94a9b17).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9960      +/-   ##
==========================================
- Coverage   91.76%   91.75%   -0.02%     
==========================================
  Files         358      358              
  Lines       16535    16535              
==========================================
- Hits        15173    15171       -2     
- Misses       1040     1041       +1     
- Partials      322      323       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrzej-stencel
Copy link
Member

I'm not sure if this change will make it easier to understand this configuration.

I had trouble understanding the original PR #9660 and I think it's because the configuration of filtering out metrics via the resource_attributes property is confusing.

Perhaps it's good to mark this newly introduced property as experimental to allow us making changes to it after we release it, when/if we find a more idiomatic way of filtering metrics by resource attributes?

@TylerHelmuth
Copy link
Member

I agree with @astencel-sumo. I've always found the include/exclude functionality in the filterprocessor confusing as well. I think it would be clearer to have any match be a drop and then supporting a negation somehow - this is the pattern the filterprocessor uses with OTTL.

@dmitryax dmitryax force-pushed the rename-resource-attr-include-exclude branch from a52cefb to 2905399 Compare April 15, 2024 19:18
@dmitryax
Copy link
Member Author

Added the experimental notice. Any suggestions are welcome

@andrzej-stencel
Copy link
Member

Thinking about this again, why not use OTTL to filter (and/or transform) a component's emitted telemetry instead of trying to come up (again?) with a declarative configuration language? Seems that we already had this problem, and we had solved it with OTTL. 🙂

Simple things like emitting or not a metric are fine by me to configure with metrics.<metric-name>.enabled, but the use case of "discarding all metrics that have a resource attribute that matches an expression" seem to be much better served by a full-fledged expression language. If I'm not mistaken, OTTL was invented exactly for these scenarios.

Similar to how users can attach Stanza operators to e.g. Filelog receiver to postprocess the telemetry before it leaves the component, we could provide a standard, cross-signal way to postprocess telemetry that could be attached to any Otelcol component (definitely to receivers, maybe connectors?).

Of course the same can be done later in the pipeline with the transform or filter processor. If we still want to allow users to filter/process the telemetry before it leaves the component, perhaps we should let the users use the standard, already familiar (hopefully!) way.

@dmitryax
Copy link
Member Author

@astencel-sumo, it's not a new language. This is a result of open-telemetry/opentelemetry-collector-contrib#25161. OTTL is only applicable to OTel data, not for filtering random slices and maps.

It may be possible to apply OTTL here, but I'm not sure how can it make the user interface simpler than what's currently proposed. I'm open to any specific suggestions.

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.
@dmitryax dmitryax force-pushed the rename-resource-attr-include-exclude branch from 2905399 to 94a9b17 Compare April 16, 2024 17:46
@andrzej-stencel
Copy link
Member

@astencel-sumo, it's not a new language. This is a result of open-telemetry/opentelemetry-collector-contrib#25161. OTTL is only applicable to OTel data, not for filtering random slices and maps.

I think we may be mixing two different things here. The one you're talking about is how to expose some (arbitrary) filtering settings in the configuration. I have no problem with this.

The other thing is that in this specific case - filtering out metrics based on their resource attributes - the filtering configuration is used to actually filter telemetry - specifically the metrics that the component emits.

Let me comment on the original issue for this: open-telemetry/opentelemetry-collector-contrib#25134 (comment).

@dmitryax dmitryax merged commit cc9e3dd into open-telemetry:main Apr 22, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 22, 2024
@dmitryax dmitryax deleted the rename-resource-attr-include-exclude branch April 22, 2024 15:45
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants