Skip to content

add ecs support #228

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

Merged
merged 5 commits into from
May 3, 2021
Merged

Conversation

kaisecheng
Copy link
Contributor

This PR adds ECS support and maps the following

Legacy ECS
cloudfront_fields [@metadata][s3][cloudfront][fields]
cloudfront_version [@metadata][s3][cloudfront][version]

Closed: #196

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
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.

Doc review: Made some suggestions in line for your consideration. Please give me a chance to re-review so that we don't break the doc build.

Do you have an example that would help clarify this info for users?

==== Event Metadata and the Elastic Common Schema (ECS)
This plugin adds cloudfront metadata to event.
By default, the value is stored in the root level.
When ECS is enabled, it is stored in the `@metadata`.
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
When ECS is enabled, it is stored in the `@metadata`.
When ECS is enabled, the value is stored in the `@metadata`.

|=======================================================================
| disabled | v1 | Availability | Description

| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | columns name of metadata
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
| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | columns name of metadata
| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file contains cloudfront metadata | columns name of metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this wording. Maybe "When the file contains cloudfront metadata?"
If I don't have the wording right, please let me know. We can work together to find the right wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my word metadata is not accurate. It should be a log file specific to CloudFront, which records the access details.
cloudfront_fields and cloudfront_version are only available for CloudFront log.
So, may be change it to "available when the file is a CloudFront log"

| disabled | v1 | Availability | Description

| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | columns name of metadata
| cloudfront_version | [@metadata][s3][cloudfront][version] | available when the file is a cloudfront metadata | version of metadata
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
| cloudfront_version | [@metadata][s3][cloudfront][version] | available when the file is a cloudfront metadata | version of metadata
| cloudfront_version | [@metadata][s3][cloudfront][version] | available when the file contains cloudfront metadata | version of metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previous

* Default value depends on which version of Logstash is running:
** When Logstash provides a `pipeline.ecs_compatibility` setting, its value is used as the default
** Otherwise, the default value is `disabled`.

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
Controls this plugin's compatibility with the
{ecs-ref}[Elastic Common Schema (ECS)].
See <<plugins-{type}s-{plugin}-ecs_metadata,ECS mapping>> for detailed information.

@@ -31,6 +31,20 @@ Files ending in `.gz` are handled as gzip'ed files.

Files that are archived to AWS Glacier will be skipped.

==== Event Metadata and the Elastic Common Schema (ECS)
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
==== Event Metadata and the Elastic Common Schema (ECS)
[id="plugins-{type}s-{plugin}-ecs_compatibility"]
==== Event Metadata and the Elastic Common Schema (ECS)

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Changes in code and specs look good.

I've left a few suggestions in docs, and we should probably get 👀 from @karenzone before we merge.

Here’s how ECS compatibility mode affects output.
[cols="<l,<l,e,<e"]
|=======================================================================
| disabled | v1 | Availability | Description
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
| disabled | v1 | Availability | Description
| ECS disabled | ECS v1 | Availability | Description

|=======================================================================
| disabled | v1 | Availability | Description

| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | columns name of metadata
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
| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | columns name of metadata
| cloudfront_fields | [@metadata][s3][cloudfront][fields] | available when the file is a cloudfront metadata | column names of metadata

ecs_compatibility_matrix(:disabled, :v1) do |ecs_select|
before(:each) do
allow_any_instance_of(described_class).to receive(:ecs_compatibility).and_return(ecs_compatibility)
# subject.register
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
# subject.register

@kaisecheng
Copy link
Contributor Author

kaisecheng commented Apr 28, 2021

Doc review: Made some suggestions in line for your consideration. Please give me a chance to re-review so that we don't break the doc build.

Do you have an example that would help clarify this info for users?

the cloudfront related fields store in invisible [@metadata] in ECS v1. The data exists but just not visible. I am afraid the example with missing fields causes more confusion. Do you think I should show the example? @karenzone

@karenzone
Copy link
Contributor

karenzone commented Apr 28, 2021

Regarding:

the cloudfront related fields store in invisible [@metadata] in ECS v1. The data exists but just not visible. I am afraid the example with missing fields causes more confusion. Do you think I should show the example? @karenzone

@kaisecheng Using examples to illustrate concepts is one of many areas where you excel. If you believe that an example would add more confusion, then I agree that we shouldn't add one.

@kaisecheng kaisecheng requested a review from karenzone April 29, 2021 12:45
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.

@kaisecheng I suggested two more fixes inline. One was the result of my error in suggesting a heading anchor that was already in use.

kaisecheng and others added 2 commits May 3, 2021 10:56
Co-authored-by: Karen Metts <[email protected]>
Co-authored-by: Karen Metts <[email protected]>
@kaisecheng kaisecheng requested a review from karenzone May 3, 2021 09:02
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.

LGTM 🎉

@kaisecheng kaisecheng merged commit 65f156c into logstash-plugins:master May 3, 2021
@kaisecheng kaisecheng mentioned this pull request Jun 24, 2021
kaisecheng added a commit that referenced this pull request Jun 24, 2021
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.

Implement ECS-Compatibility Mode
5 participants