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

ecs_compatibility: revert breaking change; keep disabled as default for 8.0 #13080

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

yaauie
Copy link
Member

@yaauie yaauie commented Jul 16, 2021

Release notes

REVERTS previously-merged breaking change in default value of pipeline.ecs_compatibility

What does this PR do?

As we close in on the availability of 8.0.0 alphas, we are reassessing which
breaking changes are necessary, and which are merely desired. And while
we would love to be in a world where ECS was on by default, and have put
substantial effort into designing an upgrade path that would be as simple as
possible, we have determined that the time may not be right to change the
default value out of under our users.

This change restores the default value for pipeline.ecs_compatibility to
disabled, ensuring pipelines will continue running in Logstash 8 as they have
in Logstash 7 without modification. We will still encourage our users to be
explicit about which behaviour they desire, and will revisit making ECS on by
default at a later date.

Why is it important/What is the impact to the user?

Prioritizes ease of upgrade to Logstash 8, while continuing to guide our users to be explicit about which behaviour they desire.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

@yaauie yaauie marked this pull request as ready for review July 20, 2021 16:18
@yaauie yaauie requested review from karenzone and robbavey July 20, 2021 16:18
@@ -70,17 +70,17 @@
# available to plugins that implement an ECS Compatibility mode for use with
# the Elastic Common Schema.
# Possible values are:
# - disabled
# - v1 (default)
# - v2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed this in docs first time around, the setting has accepted v8 for a while.

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor items:

should be v8 to be consistent with other changes.

Also, a few tests (eg conditionals spec) were updated to have ecs_compatibility => disabled set explicitly in their configurations, presumably to keep them passing, in the commit to set ECS as default. Do we have any tests that verify behaviour dependent on ecs_compatibility settings that we might expect to fail in the event of the change of default behaviour? Or that have expectations based on whether ecs_compatibility is set at the global, pipeline or plugin level? (I don't expect these to be included as part of this PR)

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions inline. Otherwise LGTM. 🚀
OK to merge when docs-ci is 🟢

docs/static/ecs-compatibility.asciidoc Outdated Show resolved Hide resolved
docs/static/ecs-compatibility.asciidoc Outdated Show resolved Hide resolved
As we continue to release plugins with ECS Compatibility modes, having this flag set will cause upgrades to _automatically_ consume breaking changes from one snapshot to another, changing the shape of data the plugin produces.
If you require stability while testing against the unreleased {ls} 8, we encourage you to opt-out globally or per-pipeline as detailed below.
NOTE: Until {ls} 8.0 and the final 7.x are released, any value for `pipeline.ecs_compatibility` other than `disabled` are considered BETA and unsupported.
As we continue to release plugins with ECS Compatibility modes, having this flag set will cause even patch-level upgrades to _automatically_ consume breaking changes in the upgraded plugins, changing the shape of data the plugin produces.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As we continue to release plugins with ECS Compatibility modes, having this flag set will cause even patch-level upgrades to _automatically_ consume breaking changes in the upgraded plugins, changing the shape of data the plugin produces.
As we continue to release plugins with ECS compatibility modes, having this flag set will cause even patch-level upgrades to _automatically_ consume breaking changes in the upgraded plugins, changing the shape of data the plugin produces.

docs/static/ecs-compatibility.asciidoc Outdated Show resolved Hide resolved
docs/static/ecs-compatibility.asciidoc Outdated Show resolved Hide resolved
@yaauie
Copy link
Member Author

yaauie commented Jul 20, 2021

@robbavey I'll address your concerns one at a time

Couple of minor items:

should be v8 to be consistent with other changes.

👍🏼 , will incude in my next round

Also, a few tests (eg conditionals spec) were updated to have ecs_compatibility => disabled set explicitly in their configurations, presumably to keep them passing, in the commit to set ECS as default.

Yes. Those specs were relying on the default value — which we warn our users against with a deprecation log — and the previous change to the default fixed those specs by making their desired behaviour explicit. I elected not to roll those changes back, as being explicit is helpful.

Do we have any tests that verify behaviour dependent on ecs_compatibility settings that we might expect to fail in the event of the change of default behaviour?

No, and that is why I have kept the explicit declarations in the above-mentioned specs.

From Logstash core's perspective, this isn't needed. This is a setting that is provided to plugins, and they each have their own tests to validate that they behave as intended with whatever the effective value of this setting ends up being. We validate here that we are providing the correct effective setting to the plugins.

Or that have expectations based on whether ecs_compatibility is set at the global, pipeline or plugin level? (I don't expect these to be included as part of this PR)

Yes. The specs around this line validate the behaviours listed. We can't really differentiate between it being set for a pipeline or globally since each pipeline effectively takes a clone of the global and provides overrides, but when it is unspecified all the way down we do warn, and we do validate that it resolves to disabled.

Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM after nit dealt with

… for 8.0.0

As we close in on the availability of 8.0.0 alphas, we are reassessing which
breaking changes are _necessary_, and which are merely _desired_. And while
we would love to be in a world where ECS was on by default, and have put
substantial effort into designing an upgrade path that would be as simple as
possible, we have determined that the time may not be right to change the
default value out of under our users.

This change restores the default value for `pipeline.ecs_compatibility` to
`disabled`, ensuring pipelines will continue running in Logstash 8 as they have
in Logstash 7 without modification. We will still encourage our users to be
explicit about which behaviour they desire, and will revisit making ECS on by
default at a later date.
@yaauie yaauie force-pushed the ecs-disabled-in-v8 branch from 494af39 to fd7d3e4 Compare July 20, 2021 20:14
@yaauie yaauie merged commit 68f3cf3 into elastic:master Jul 20, 2021
@yaauie yaauie deleted the ecs-disabled-in-v8 branch July 20, 2021 21:45
kares added a commit to kares/logstash that referenced this pull request Aug 23, 2021
* master:
  Update releases list (elastic#13149)
  Bundler: freeze lockfile on run, and "normalize" platform on plugin changes (elastic#13015)
  Doc: Move shared region tags for breaking changes to 8.0.0 content (elastic#13131)
  Update Snakeyaml version to 1.29 (elastic#13129)
  Doc: Add breaking changes to breaking changes page (elastic#13130)
  Added faraday-* and ruby2_keywords notices to licences list (elastic#13126)
  Release notes draft for 8.0.0-alpha1 (elastic#13098)
  valid variable expansion for the batch files (elastic#12912)
  Doc: Enhance and expand DLQ docs (elastic#12959)
  Update releases list with 7.13.4 (elastic#13088)
  Doc:Fix typo and adjust keystore text (elastic#12779)
  doc: add pipeline.ecs_compatibility docs (elastic#12421)
  ecs_compatibility: revert breaking change; keep `disabled` as default for 8.0.0 (elastic#13080)
  Doc: Add kafka schema registry setting to troubleshooting (elastic#13073)
  [Test] Fix Unix acceptance tests (elastic#13071)
  Move retrieval of Stack version from Gradle's configuration to execution (elastic#13042)
  Update releases list with 6.8.17 and 7.13.3 (elastic#13041)
  Docs: keep elastic_app_search output meta-data (elastic#13048)
  Fix LS benchmarking tool to work with releases >= 7.10.0 (elastic#13052)
  Revert "Change Gradle's :logstash-integration-tests:integrationTests task to depends on copyES (elastic#12847)" (elastic#13045)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants