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

Implement ECS-Compatibility Mode #17

Closed
yaauie opened this issue Feb 26, 2020 · 5 comments · Fixed by #18
Closed

Implement ECS-Compatibility Mode #17

yaauie opened this issue Feb 26, 2020 · 5 comments · Fixed by #18

Comments

@yaauie
Copy link
Contributor

yaauie commented Feb 26, 2020

This is a stub issue, and needs to be fleshed out with details specific to
this plugin.


As a part of the effort to make plugins able to run in an ECS-Compatible manner
by default in an upcoming release of Logstash, this plugin needs to either
implement an ECS-Compatibility mode or certify that it does not implicitly use
fields that conflict with ECS.

@andsel
Copy link
Contributor

andsel commented Jun 15, 2021

The plugin adds a clock field which could be valued to Time.now.to_i or to a counter, the selection is done by the value of message.

  • if message == epoch then add Time.now
  • if message == sequence then add the counter
  • else add the message field as it is.

Regarding to ECS, the case of the epoch selector is exactly what @timestamp is: https://www.elastic.co/guide/en/ecs/1.10/ecs-base.html#field-timestamp .

The case of sequence is not linked to any concept in ECS but to be future proof we could move it in a [@metadata][sequence] field.

The message is already compatible with ECS: https://www.elastic.co/guide/en/ecs/1.10/ecs-base.html#field-timestamp

@andsel
Copy link
Contributor

andsel commented Jun 15, 2021

@yaauie Logstash has already the @timestamp but it's in yyyy-MM-dd'T'HH:mm:ss.SSSZ format and not as integer (the number of seconds). Do you think we should change what the plugin output when the epoch is selected, from integer to normal timestamp format, or it's better to create another @metadata's sub field ?

@yaauie
Copy link
Contributor Author

yaauie commented Jun 21, 2021

Summarizing outside discussion:

  1. event.sequence looks like a good home for an integer field representing sequential ordering (whether by epoch or monotonically incrementing sequence number) -- HT @kares
  2. If we can do so, I would like to stop overloading message to change behaviour, and I think the opt-in nature of ECS compatibility mode gives us a good opportunity to stop carrying it forward.
    • Let's introduce a new option to control this behaviour - maybe sequence with values epoch,increment, and none?
    • in non-ECS mode, we continue intercept magical values for message during register, using them to:
      • populate our new sequence if it wasn't given,
      • unset @message so that we don't create events with {"message":"epoch"}

@andsel
Copy link
Contributor

andsel commented Jun 22, 2021

in non-ECS mode, unset @message so that we don't create events with {"message":"epoch"}

I've the suspect that we potentially break something doing this.
Actually, if message has valid value we could end in two kind of events:

{ "message": "epoch",
  "clock": 1233254534

or

{ "message": "sequence",
  "clock": 1

In both cases the event contains itself the type of the counter. If we unset the message in non-ECS mode we could potentially break some pipelines that leverages that field to do some choices.

We could introduce the new config option (name it sequence_type), in non-ECS mode I would leave it as it's today plus warning the user, through log message, that he should use sequence_type instead of message, giving precedence to sequence_type if it's set. In non-ECS it still create the clock field.

In ECS mode:

  • the message is not anymore considered to select the sequence but only sequence_type
  • the message is unset if contains epoch or increment
  • event.sequence field is created and no more clock field

Do you think the plugin should also put a metadata or a tag to say which kind of sequence the event.sequence is representing (epoch or sequence) ?

@yaauie
Copy link
Contributor Author

yaauie commented Jun 23, 2021

If we unset the message in non-ECS mode we could potentially break some pipelines that leverages that field to do some choices

Currently, as implemented without ECS mode, if a user specifies message => epoch or message => sequence, they get events that have no message, only clock and host; this behaviour should remain in non-ECS mode.

In ECS mode, I prefer sequence as the new option with valid values epoch, increment, and none (default). It is a little more terse than sequence_type, and I favor compact configuration where we can make it possible.

  1. change implementation to change behaviour based on value of sequence option (@sequence ivar), including a message field if @message is present.
  2. during registration in non-ECS mode when sequence was not provided, intercept magic "message" values, setting @sequence and unsetting @message to emulate current behaviour.

@andsel andsel closed this as completed in #18 Jul 1, 2021
andsel added a commit that referenced this issue Jul 1, 2021
This PR does 3 things:
* enable ECS compatibility, replacing the clock field with [event][sequence] when ECS is enabled
* introduces a new option named sequence to configure the kind of counter used in the "clock/sequence" field
* unrelated to the context of ECS it switches the creation of the Event to the event_factory mixin

In non ECS mode the sequence setting takes precedence over message and if message contains a value other then epoch or sequence it includes also the message field in the generated event.

In ECS mode only the sequence setting is considered to select the kind of counter, if message contains epoch or sequence its simply ignored and no message field is present in the event, otherwise message is part of the event together with the [event][sequence]

Fixes #17

Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
Co-authored-by: Karol Bucek <kares@users.noreply.github.com>
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 a pull request may close this issue.

2 participants