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.

Sign up for GitHub

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

Add transform to spec #307

Merged
merged 23 commits into from
Jul 6, 2022
Merged

Add transform to spec #307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f5100f9
add to spec and test
eyalkraft Mar 30, 2022
032787a
update changelog
eyalkraft Mar 30, 2022
4b51706
fix changelog
eyalkraft Mar 30, 2022
291a75e
revert newline change
eyalkraft Mar 31, 2022
20f7a09
enforce subfolders and default.json naming
eyalkraft Mar 31, 2022
f8b57ac
add endpoint transforms as good examples
eyalkraft Apr 3, 2022
5095360
allow numbers in transform dir name
eyalkraft Apr 3, 2022
6ca6997
Merge branch 'main' into add-transform-to-spec
eyalkraft Apr 3, 2022
1b6687c
Merge branch 'main' into add-transform-to-spec
eyalkraft Jun 9, 2022
fd35caa
add dest index template
eyalkraft Jun 9, 2022
b63ec41
Merge branch 'main' into add-transform-to-spec
eyalkraft Jun 21, 2022
594115c
fix review comments
eyalkraft Jun 21, 2022
48d0bc6
wrap timestamp
eyalkraft Jun 21, 2022
8d7282c
add fields
eyalkraft Jun 22, 2022
32f5ed1
fix multifields
eyalkraft Jun 22, 2022
1e34688
fix nits
eyalkraft Jun 23, 2022
dd07c1a
add start flag
eyalkraft Jun 26, 2022
940629f
add naming convention to description
eyalkraft Jun 26, 2022
5a604bc
add transform validations
eyalkraft Jun 26, 2022
e736216
update index template naming
eyalkraft Jun 27, 2022
9b008f3
update transform naming
eyalkraft Jun 27, 2022
84abd35
update example
eyalkraft Jun 27, 2022
c06c5c7
update description with recommended source and destination index naming
eyalkraft Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions test/packages/good/elasticsearch/transform/good_transform_abc_1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"source": {
"index": "kibana_sample_data_ecommerce",
"query": {
"term": {
"geoip.continent_name": {
"value": "Asia"
}
}
}
},
"pivot": {
"group_by": {
"customer_id": {
"terms": {
"field": "customer_id"
}
}
},
"aggregations": {
"max_price": {
"max": {
"field": "taxful_total_price"
}
}
}
},
"description": "Maximum priced ecommerce data by customer_id in Asia",
"dest": {
"index": "kibana_sample_data_ecommerce_transform1",
"pipeline": "add_timestamp_pipeline"
},
"frequency": "5m",
"sync": {
"time": {
"field": "order_date",
"delay": "60s"
}
},
"retention_policy": {
"time": {
"field": "order_date",
"max_age": "30d"
}
}
}
3 changes: 3 additions & 0 deletions versions/1/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,6 @@
- description: Introduce "value" property for constant_keyword fields
type: enhancement
link: https://github.com/elastic/package-spec/pull/194
- description: Add Elasticsearch transform support
mtojek marked this conversation as resolved.
Show resolved Hide resolved
type: enhancement
link: https://github.com/elastic/package-spec/pull/307
10 changes: 10 additions & 0 deletions versions/1/elasticsearch/spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,13 @@ spec:
pattern: '^.+\.json$'
contentMediaType: "application/json"
required: false
- description: Folder containing Elasticsearch Transforms
type: folder
name: transform
required: false
contents:
- description: An Elasticsearch transform file
# https://www.elastic.co/guide/en/elasticsearch/reference/current/transforms.html
type: file
contentMediaType: "application/json"
pattern: '^.+\.json$'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should apply any naming requirements? For example, to prepend a package name to the file. Is it possible that another package can override this transform?

Copy link
Contributor Author

@eyalkraft eyalkraft Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure but I think the filename doesn't effect the created Transform at all.
As you can see in our integration (works locally) we have two different transforms in different directories with the same file name (default.json) and they don't override each other.
Probably @joshdover could explain better the exact meaning and implications the filenames and the containing directory names have.
endpoint security integration transforms look the same.

Copy link
Contributor

@mtojek mtojek Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

endpoint security integration transforms look the same.

From the spec perspective, all Endpoint security integration transforms are illegal as they haven't been documented. This is the reason why you need to deal with it now. We will enable the strict package validation soon and it will block the publishing of packages with undocumented (= illegal) features. cc @pzl

I'm not sure but I think the filename doesn't effect the created Transform at all.
As you can see in our integration (works locally) we have two different transforms in different directories with the same file name (default.json) and they don't override each other.

Let me elaborate here, what will happen if we have another package foobar with exact transforms and directories? When we install it, will it overwrite your transforms?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @pzl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me elaborate here, what will happen if we have another package foobar with exact transforms and directories? When we install it, will it overwrite your transforms?

To my understanding the answer is no.
Seems like the id of the transform is a result of both the integration package name and the transform directory name.

Screen Shot 2022-03-31 at 15 04 25

Additionally after having a conversation with @CohenIdo about the naming of the transform files I understood he was instructed to name them default.json so I'll change the spec to reflect this requirement.

Anyways I'm sure Josh will shed more light over this topic.

Copy link

Choose a reason for hiding this comment

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

@eyalkraft thanks for doing this, it's something I should have done some time back when my team originally added transforms.

@mtojek

I'm not sure but I think the filename doesn't effect the created Transform at all.
Let me elaborate here, what will happen if we have another package foobar with exact transforms and directories? When we install it, will it overwrite your transforms?

Currently, when transforms are installed, the package name will be prepended (along with the directory path) to actual name of the transform. Thus, it doesn't matter what the definition .json file is named. So, as long as the packages themselves are named differently, I don't think there will be any collisions. Here is what our transforms are named when installed. (@pzl let me know if I have anything wrong here)

image

That being said, I am OK with any naming convention we want to enforce and we can update the names of files to match.

We will enable the strict package validation soon and it will block the publishing of packages with undocumented (= illegal) features.

For clarity, adding this PR to the package spec will "document" the transform feature and will no longer make it illegal, correct?

Further, let us know when we can test against strict package enforcement so we can fix any other problems on our end and be prepared for when it is live. I would ask that we can publish packages as is for the next couple weeks so that we can push our package with changes for the 8.2 release and in case we need to push anything additional to fix bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I am OK with any naming convention we want to enforce and we can update the names of files to match.

That's the killer argument for this issue. I guess we can resolve the conversation. Thanks for jumping in, Kevin.

For clarity, adding this PR to the package spec will "document" the transform feature and will no longer make it illegal, correct?

Yes, that's the idea.

Further, let us know when we can test against strict package enforcement so we can fix any other problems on our end and be prepared for when it is live. I would ask that we can publish packages as is for the next couple weeks so that we can push our package with changes for the 8.2 release and in case we need to push anything additional to fix bugs.

@eyalkraft It would be great if we can copy Endpoint's transforms here as test cases to make sure they are aligned with new spec requirements. @kevinlog What would be the best source to copy from?

Copy link

@kevinlog kevinlog Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@eyalkraft @mtojek

@ eyalkraft It would be great if we can copy Endpoint's transforms here as test cases to make sure they are aligned with new spec requirements. @ kevinlog What would be the best source to copy from?

Either of the definitions here are good: https://github.com/elastic/endpoint-package/tree/master/package/endpoint/elasticsearch/transform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added both