Skip to content

Conversation

@deepdatta
Copy link
Contributor

Description

Add documentation for ecs compatibility

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has documentation added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@deepdatta deepdatta requested a review from a team as a code owner August 24, 2022 21:16
README.md Outdated
}
```
### ecs_compatibility
ECS compatibility for V8 was added in 1.3.0. For more details on ECS support refer to this [documentation](docs/ecs_compatibility.md).
Copy link
Contributor

@sshivanii sshivanii Aug 25, 2022

Choose a reason for hiding this comment

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

Include the acronym's full form with the link Elastic Common Schema(ECS) so that the reader knows what you're referring to.
Nit: Reword it Version 1.3.0.
Also, I noticed in the release notes for version 1.3.0 there is no mention of ECS compatibility but we're mentioning here it was added in that version. We should update the release notes for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Good call on expanding it the first time mentioned and the link.

README.md Outdated
### ecs_compatibility
ECS compatibility for V8 was added in 1.3.0. For more details on ECS support refer to this [documentation](docs/ecs_compatibility.md).

## Detailed Documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

This section seems out of place as detailed documentation and can be removed. If you still want to include it, this can be added as a link Ship events to OpenSearch under Project resources

README.md Outdated
}
}
```
### ecs_compatibility
Copy link
Contributor

@sshivanii sshivanii Aug 25, 2022

Choose a reason for hiding this comment

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

Might be worthwhile adding a new section Support for ECS Compatibility otherwise it looks like it's a part of Configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe name this ECS Compatibility?

```
[2022-08-23T00:01:53,366][WARN ][logstash.outputs.opensearch][main][a36555c6fad3f301db8efff2dfbed768fd85e0b6f4ee35626abe62432f83b95d] Could not index event to OpenSearch. {:status=>400, :action=>["index", {:_id=>nil, :_index=>"ecs-logstash-2022.08.23", :routing=>nil}, {"@timestamp"=>2022-08-22T15:39:18.142175244Z, "@version"=>"1", "server"=>"remoteserver.com", "message"=>"Doc1"}], :response=>{"index"=>{"_index"=>"ecs-logstash-2022.08.23", "_id"=>"CAEUyYIBQM7JQrwxF5NR", "status"=>400, "error"=>{"type"=>"mapper_parsing_exception", "reason"=>"object mapping for [server] tried to parse field [server] as object, but found a concrete value"}}}}
```
## How to make things compatible
Copy link
Contributor

Choose a reason for hiding this comment

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

Reword to How to ensure ECS compatibility

[2022-08-23T00:01:53,366][WARN ][logstash.outputs.opensearch][main][a36555c6fad3f301db8efff2dfbed768fd85e0b6f4ee35626abe62432f83b95d] Could not index event to OpenSearch. {:status=>400, :action=>["index", {:_id=>nil, :_index=>"ecs-logstash-2022.08.23", :routing=>nil}, {"@timestamp"=>2022-08-22T15:39:18.142175244Z, "@version"=>"1", "server"=>"remoteserver.com", "message"=>"Doc1"}], :response=>{"index"=>{"_index"=>"ecs-logstash-2022.08.23", "_id"=>"CAEUyYIBQM7JQrwxF5NR", "status"=>400, "error"=>{"type"=>"mapper_parsing_exception", "reason"=>"object mapping for [server] tried to parse field [server] as object, but found a concrete value"}}}}
```
## How to make things compatible
* As mentioned at the beginning, the plugins in the pipeline that create the events like the `input` and `codec` plugins should all use ECS defined fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where was this mentioned? Might be worth mentioning the exact section.

* You can use your own custom template in the plugin using the `template` and `template_name` configs.
[According to this](https://www.elastic.co/guide/en/ecs/current/ecs-faq.html#type-interop) some field types can be changed while staying compatible.

As a last resort the `ecs_compatibility` of the logstash-output-opensearch can be set to `disabled`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to further clarify where this ecs_compatibility is defined and how it's disabled by including an example.



_______________
## References
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented about this on line 95.

Signed-off-by: Deep Datta <deedatta@amazon.com>
@deepdatta
Copy link
Contributor Author

added new commit with changes based on the reviews

@deepdatta deepdatta merged commit 3ea2ba7 into opensearch-project:main Sep 1, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 1, 2022
* Add a documentation for ecs compatibility

Signed-off-by: Deep Datta <deedatta@amazon.com>
(cherry picked from commit 3ea2ba7)
deepdatta pushed a commit that referenced this pull request Sep 1, 2022
* Add a documentation for ecs compatibility

Signed-off-by: Deep Datta <deedatta@amazon.com>
(cherry picked from commit 3ea2ba7)
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.

4 participants