[processor/filter] Deprecate non-ottl configuration #18613
Conversation
Foresight Summary
View More Details⭕ build-and-test-windows workflow has finished in 5 seconds (40 minutes 45 seconds less than
|
| Job | Failed Steps | Tests | |
|---|---|---|---|
| windows-unittest-matrix | - 🔗 | N/A | See Details |
| windows-unittest | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 3 minutes 25 seconds and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changelog | - 🔗 | N/A | See Details |
✅ telemetrygen workflow has finished in 4 minutes 58 seconds (⚠️ 1 minute 51 seconds more than main branch avg.) and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| publish-stable | - 🔗 | N/A | See Details |
| publish-latest | - 🔗 | N/A | See Details |
| build-dev | - 🔗 | N/A | See Details |
✅ check-links workflow has finished in 9 minutes 14 seconds (⚠️ 6 minutes 38 seconds more than main branch avg.) and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changed files | - 🔗 | N/A | See Details |
| check-links | - 🔗 | N/A | See Details |
✅ prometheus-compliance-tests workflow has finished in 13 minutes 28 seconds (⚠️ 4 minutes 39 seconds more than main branch avg.) and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| prometheus-compliance-tests | - 🔗 | ✅ 21 ❌ 0 ⏭ 0 🔗 | See Details |
✅ e2e-tests workflow has finished in 20 minutes 30 seconds (⚠️ 3 minutes 42 seconds more than main branch avg.) and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| kubernetes-test | - 🔗 | N/A | See Details |
❌ load-tests workflow has finished in 47 minutes 50 seconds (⚠️ 30 minutes 48 seconds more than main branch avg.) and finished at 14th Feb, 2023. 1 job failed. There are 2 test failures.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| loadtest (TestIdleMode) | - 🔗 | ✅ 1 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetric10kDPS|TestMetricsFromFile) | - 🔗 | ✅ 6 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestBallastMemory|TestLog10kDPS) | - 🔗 | ✅ 18 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetricResourceProcessor|TestTrace10kSPS) | - 🔗 | ✅ 12 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceAttributesProcessor) | - 🔗 | ✅ 3 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) | - 🔗 | ✅ 10 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) | Loadtest 🔗 | ✅ 6 ❌ 2 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
✅ build-and-test workflow has finished in 1 hour 32 minutes 59 seconds (⚠️ 27 minutes 32 seconds more than main branch avg.) and finished at 14th Feb, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| integration-tests | - 🔗 | ✅ 55 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, internal) | - 🔗 | ✅ 561 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, extension) | - 🔗 | ✅ 537 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, processor) | - 🔗 | ✅ 1509 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-metrics | - 🔗 | ✅ 2 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, exporter) | - 🔗 | ✅ 2455 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, processor) | - 🔗 | ✅ 1509 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-traces | - 🔗 | ✅ 17 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, other) | - 🔗 | ✅ 4684 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-0) | - 🔗 | ✅ 2574 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, exporter) | - 🔗 | ✅ 2455 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-1) | - 🔗 | ✅ 1928 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.18, receiver-1) | - 🔗 | ✅ 1928 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, other) | - 🔗 | ✅ 4684 ❌ 0 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
| lint-matrix (receiver-0) | - 🔗 | N/A | See Details |
| lint-matrix (processor) | - 🔗 | N/A | See Details |
| lint-matrix (extension) | - 🔗 | N/A | See Details |
| lint-matrix (receiver-1) | - 🔗 | N/A | See Details |
| lint-matrix (exporter) | - 🔗 | N/A | See Details |
| lint-matrix (internal) | - 🔗 | N/A | See Details |
| checks | - 🔗 | N/A | See Details |
| check-collector-module-version | - 🔗 | N/A | See Details |
| check-codeowners | - 🔗 | N/A | See Details |
| build-examples | - 🔗 | N/A | See Details |
| lint-matrix (other) | - 🔗 | N/A | See Details |
| lint | - 🔗 | N/A | See Details |
| unittest (1.19) | - 🔗 | N/A | See Details |
| unittest (1.18) | - 🔗 | N/A | See Details |
| cross-compile (darwin, amd64) | - 🔗 | N/A | See Details |
| cross-compile (darwin, arm64) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm64) | - 🔗 | N/A | See Details |
| cross-compile (windows, 386) | - 🔗 | N/A | See Details |
| cross-compile (linux, 386) | - 🔗 | N/A | See Details |
| cross-compile (linux, amd64) | - 🔗 | N/A | See Details |
| cross-compile (linux, ppc64le) | - 🔗 | N/A | See Details |
| cross-compile (windows, amd64) | - 🔗 | N/A | See Details |
| build-package (deb) | - 🔗 | N/A | See Details |
| build-package (rpm) | - 🔗 | N/A | See Details |
| windows-msi | - 🔗 | N/A | See Details |
| publish-check | - 🔗 | N/A | See Details |
| publish-dev | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
*You can configure Foresight comments in your organization settings page.
This comment was marked as outdated.
This comment was marked as outdated.
|
@Aneurysm9 when did we start releasing the collector weekly? I thought we released every other week, on Mondays. |
|
What is motivating this change? Is there a tracking issue that talks about migrating the filter processor to using strictly OTTL? I personally found the legacy filter options more approachable. If a user wanted to use OTTL shouldn't we point them to the transform processor instead? |
You're right. I think we discussed moving to weekly releases along with the move to Mondays and I was conflating the two when we only did one. Either way, this is a significant change and that's an aggressive timeline for something we'll be first communicating to users with this change. |
| component: filterprocessor | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Deprecation warning for all non-OTTL configuration options. Use OTTL configuration options instead. All non-OTTL configuration options will be removed in 0.74.0. |
There was a problem hiding this comment.
I agree with @Aneurysm9 that v0.74.0 is too aggressive, should we just say 'in a future version' in this PR? This component is in alpha, but it's quite old so it's probably widely used.
| oCfg := cfg.(*Config) | ||
| if oCfg.Metrics.Include != nil || oCfg.Metrics.Exclude != nil { | ||
| set.Logger.Warn( | ||
| "The metric `include` and `exclude` configuration options have been deprecated and will be removed in 0.74.0. Use `metric` instead.", |
There was a problem hiding this comment.
Is there anything we can link here to help end-users understand how their old configuration should be rewritten in OTTL statements? The docs on how to use metric (same for log_record and traces.span) tell you to use OTTL, but if you are completely new to OTTL I think it would help to have a bit more hand-holding here (maybe a link to an issue where we write some migration instructions?).
There was a problem hiding this comment.
maybe a link to an issue where we write some migration instructions?
I can add documentation on how to convert each signal's existing configuration options into OTTL. I could do that right in the README instead of an issue, what do you think?
There was a problem hiding this comment.
Is mechanical conversion possible? If so, should we provide a confmap.Converter implementation that can automatically handle the conversion?
There was a problem hiding this comment.
@Aneurysm9 I'm not familiar with that interface yet, but it is possible that a conversion could be written. I don't think it would be trivial, but hopefully reusable for other components that implement internal/filter config structs.
If we go that route, I would vote it not be a permanent solution. I would still want to eventually deprecate the old configuration style and instead support declarative syntax via an OTTL-specific declarative format long term.
There was a problem hiding this comment.
For this PR, I am fine with anything that an inexperienced user can grasp easily without having to search through the docs. I think the Converter idea sounds nice, but I don't think we should block this PR on that work
There was a problem hiding this comment.
I think if the converter will not be permanent then the value of developing one may be moot. I could see the argument of a converter doing more harm than good also. Users may end up relying on the converter that will end up being removed. It may be better to invest in a migration guide and ensure that OTTL documentation is ready for more users/components to rely on.
The motivating change is to simplify the filterprocessor's configuration, which at the moment is a mix of capabilities that change based on the signal type being filtered. The filterprocessor was chosen as the defacto solution for dropping data, so a In the past we only had what was in By unifying on OTTL we'll have a solution that has access to all fields on all signals and provides a unified filtering configuration experience no matter which signal is being filtered. It also provides path for easily adding features going forward. I anticipate many components in the future utilizing OTTL for condition. All that said, I admit I've not done a good job of writing out a roadmap for the filterprocessor (or the role OTTL should play in the collector as a whole). I can write up those issues for further discussion, and I will add an agenda item to the SIG meeting this week for the filterprocessor specifically for further discussion.
I am ok extending the transition period, but I would like to give a definitive timeline. |
What about 6 releases (so ~3 months)? Would that sound reasonable @Aneurysm9? |
| @@ -161,6 +168,7 @@ var severityToNumber = map[string]plog.SeverityNumber{ | |||
| var errInvalidSeverity = errors.New("not a valid severity") | |||
|
|
|||
| // logSeverity is a type that represents a SeverityNumber as a string | |||
| // Deprecated: [v0.72.0] use LogConditions instead | |||
There was a problem hiding this comment.
I would add to this comment the version we plan on removing this in, to prevent someone from removing it too quickly :)
|
Based on our discussion in the SIG meeting today I am going to close this for now and open a different PR with the auto-conversion/logging solution we discussed. More details in the issue. |
Description:
Deprecates all non-OTTL configuration options. Since this is a pretty serious deprecation, I'm planning for 0.74.0 to be the removal release. This give users 2 full release cycles (4 weeks), to update their configs.
I added warnings during initialization to call out the deprecations.
Issue
Works towards #18642
Testing:
Ran the collector locally to see the warnings.
Documentation:
Updated README to call our deprecation.