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

API: support ECS major version number #3

Merged

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Jul 2, 2020

Prior to this change, ECS Compatibility had two modes: false and true. As ECS is expecting to ship breaking changes to its schema along-side Elastic Stack 8.0 (elastic/ecs#839), it is becoming clear that plugins may stand to benefit from knowing the desired major schema version.

As this is an API support adapter, there are two usage perspectives to consider:

Plugin Implementation

This change-set allows plugin maintainers to supply implementations for specific ECS major versions (which may be shared), while still allowing the pre-ECS behaviour to be selected. It does so by publicly implementing an ecs_compatibility method that returns a Symbol containing either :disabled or a v-prefixed integer representing a major ECS version (e.g., :v1).

Generally, plugins will implement something like the following in their register method:

  def register
    case ecs_compatibility
    when :disabled
      # legacy behaviour
    when :v1
      # ECS v1 behaviour
    else
      fail(NotImplementedError, "ECS v#{ecs_compatibility} is not supported by this plugin.")
    end
  end

The API uses a Symbol to represent the current mode to accomplish several goals:

  • this adapter is meant as a stand-in to add functionality only to Logstashes that don't already provide it, so its API cannot include types that are introduced here (doing so would create a circular dependency).
  • this adapter does not constrain ECS versions more than absolutely necessary, freeing us from yet-another-place to maintain a list, and allowing plugins to define what they implement.
  • encourages implementations to use a simple case statement to define behaviour, improving usability (e.g., when the 1.x -> 2.x schema change does not break a specific implementation, the plugin can use when :v1, :v2 to capture both major versions 1 and 2).

Plugin Usage in Logstash Pipelines

To explicitly configure a plugin using this adapter to disable ECS:

filter {
  transmogrify {
    ecs_compatibility => disabled
  }
}

Or, to explicitly request version 1 of ECS:

filter {
  transmogrify {
    ecs_compatibility => v1
  }
}

Notes:

  • Out of scope: including the minor version of ECS schema in the API exponentially increases the complexity of this adapter gem, which is only meant to define the API and provide a stand-in implementation.
  • PR Includes other refactor/cleanup work, but in separate commits previously LGTM'd in Prepare 1.0.0 #2 (sorry 😩)
  • This change-set breaks the API, which is still possible as we are pre-1.0.0.

explicitly discourage use of `@ecs_compatibility` instance variable, since
it is not clear how Logstash Core will implement its ECS-Compatibility mode
@yaauie
Copy link
Contributor Author

yaauie commented Jul 2, 2020

ALTERNATIVES:

  • ecs_compatiblity method could always return a symbol that it derives from the possibly-integer input (:disabled, v1)
  • usage could constrain to disabled or a v-prefixed integer to increase readability.

EDIT: upon further consideration, using v-prefixed integers greatly increases the clarity of intent, so I have modified this PR to incorporate these alternatives

Modifies API to make the ECS Schema major version the primary unit to target,
in order to provide a path to support the ECS Schema v2 that is targeted to
release along-side Elastic Stack 8.0

Plugin implementors are now supplied with `LegacyAdapter#ecs_compatibility`
that produces a `Symbol` representing the desired ECS-Compatibility mode
(either `:disabled` or a v-prefixed integer e.g., `:v1`).

Plugin configuration in a pipeline definition now can specify `disabled`
or a v-prefixed integer (e.g., `v1`).
@yaauie yaauie force-pushed the api-support-ecs-major-version-number branch from 7b213d8 to ad8a05c Compare July 2, 2020 20:46
@kares kares assigned kares and unassigned elasticsearch-bot Jul 7, 2020
Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

👍 very much on where this is going, and that we're forward thinking about ECS ❤️

There's a fly 🪲 in my head regarding the 'previous' way of doing configuration, let me explain:

I do like ecs_compatibility => false or ecs_compatibility => 1.0 since it's kind of 'consistent' with an on/off flag and also with ECS versioning. No need to have to think much about what are the supported options.
For users trying to configure a plugin, given current ECS is 1.5 what do they put here: v1 looks like it should do but might feel confusing.

I know minor versions are out-of-scope atm but how will this look if at some point a plugin would need to handle ECS minor level changes (just-in-case) ?
I mean, there's going to be v1 but than the format slightly changes to v1.5.

Haven't put as much thought into this but naively seems like parsing numbers and using ranges we might be able to accomplish the same and allow for later flexibility, if needed? e.g.

  def register
    case ecs_compatibility
    when :disabled, :false
      # legacy behaviour
    when 1...1.5 # >= 1.0 (<1.5)
      # previous ECS 1.0 behaviour
    when 1.5...2, :true # >= 1.5 (< 2.0)
      # default ECS (>= 1.5) behavior
    else
      fail(NotImplementedError, "ECS #{ecs_compatibility} is not supported by this plugin.")
    end
  end

@yaauie
Copy link
Contributor Author

yaauie commented Jul 7, 2020

Out of scope: including the minor version of ECS schema in the API exponentially increases the complexity of this adapter gem, which is only meant to define the API and provide a stand-in implementation.

I'll expound on this a bit.

Supporting minor declarations and minor implementations at worst requires lock-step releases of all ECS-supporting plugins with the ECS schema, and at best provides little benefit. Consider:

  • request that plugin Z be run with ecs_compatibility => "1.97"
  • plugin Z is known to implement 1.33
    • plugin Z can reject the request to run anything greater than 1.33
      • requires lock-step releases of all ECS-implementing plugins with the ECS schema
    • plugin Z can optionally warn that it is running with 1.33, but run anyway
      • later, plugin Z is updated to add a new field from ECS 1.97 in an additive, non-breaking manner
      • user consumes updated plugin (e.g., by minor upgrade of Logstash)
      • warning goes away and user gets the new field in events.

Since ECS is Semantically Versioned, requesting a specific major or implementing a specific major should be sufficient. The added benefit of being able to optionally warn someone that a plugin doesn't support the exact version they are requesting is counter-balanced by the fact that the warning will be lost in the flood of other plugins's similar warnings when most minor releases won't affect most plugins.

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

Thanks for explaining these thoroughly.
Makes sense to really not care about minors.

@yaauie yaauie merged commit f43148c into logstash-plugins:master Jul 7, 2020
@yaauie yaauie deleted the api-support-ecs-major-version-number branch July 7, 2020 21:43
@yaauie yaauie mentioned this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants