Skip to content

Add support for ML models in elastic-package dump #1550

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 2 commits into from
Nov 9, 2023

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Nov 8, 2023

Some packages can include ML models, include them in dumps when available.

@jsoriano jsoriano requested a review from a team November 8, 2023 21:01
@jsoriano jsoriano self-assigned this Nov 8, 2023
"estimated_operations": 0,
"license_level": "platinum",
"description": "Model used to detect domain generation algorithm (DGA) activity in your network data.",
"compressed_definition": "//REDACTED//",
Copy link
Member Author

Choose a reason for hiding this comment

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

Manually removed to avoid having several megabytes of binaries here.

recordURL.Path = r.URL.Path
recordURL.RawQuery = r.URL.RawQuery

req, err := http.NewRequest(r.Method, recordURL.String(), nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix to correctly record responses of urls with query parameters. I can move this small change to a different PR if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM to add it here, and this file should be used just in testing.

Just one doubt, why not using directly r.URL.String() as parameter ? Was it for removing headers or other fields ?

req, err := http.NewRequest(r.Method, r.URL.String(), nil)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, IIRC the idea is to remove anything that is not needed from the original request. Hosts, authentication and so on are provided by the elasticsearch client.
This has limitations, as we have found here. If we had things based on headers it would fail too. Also if we would like to capture requests with payload.
We may continue improving it as we need it.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @jsoriano

recordURL.Path = r.URL.Path
recordURL.RawQuery = r.URL.RawQuery

req, err := http.NewRequest(r.Method, recordURL.String(), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM to add it here, and this file should be used just in testing.

Just one doubt, why not using directly r.URL.String() as parameter ? Was it for removing headers or other fields ?

req, err := http.NewRequest(r.Method, r.URL.String(), nil)

Comment on lines +39 to +40
// Wildcard may be too wide, but no other thing is available at the moment.
api.ML.GetTrainedModels.WithModelID(fmt.Sprintf("%s_*", packageName)),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jsoriano jsoriano merged commit 23ab9bd into elastic:main Nov 9, 2023
@jsoriano jsoriano deleted the dump-ml-jobs branch November 9, 2023 17:20
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