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

[ML] Updates APM Module to Work with Service Maps #70361

Merged
merged 4 commits into from
Jul 2, 2020

Conversation

blaklaybul
Copy link
Contributor

Summary

This PR contains an updated ML job for APM's service map anomaly detection integration that allows users to enable machine learning on all services and transaction types in a specified environment.

The module id remains unchanged, but the job and datafeed ids have been updated, so there should be no issues for existing integration users looking to enable the service maps integration. Existing users can choose to run the old job alongside this new one, but we should provide guidance for those users looking to offload the existing job.

module: apm_transaction
job: high_mean_transaction_duration

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

"query": {
"bool": {
"filter": [
{ "term": { "processor.event": "transaction" } },
{ "term": { "transaction.type": "request" } }
Copy link
Member

Choose a reason for hiding this comment

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

Was the previous job tied to transaction.type=request? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

That change happened in #30820 - does that mean ML did not work for other transaction types for the past 1.5 years?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, if the jobs were created via the ML job creation process, then the datafeed filtered for "transaction.type": "request".

@@ -7,7 +7,7 @@
"bool": {
"filter": [
{ "term": { "processor.event": "transaction" } },
{ "term": { "transaction.type": "request" } }
{ "exists": { "field": "transaction.duration.us" } }
Copy link
Member

@sorenlouv sorenlouv Jul 1, 2020

Choose a reason for hiding this comment

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

Given the following request to create a new ML job:

POST /api/ml/modules/setup/apm_transaction
{
  // tagging the job with service.environment
  "custom_settings": {
    "service.environment": "production"
  },
  // tagging the job with app name
  "groups": ["apm"],
  // specifying the indicies to query
  "indexPatternName": "apm-*-transaction-*",
  // create job and start immediately
  "startDatafeed": true,
  // limit job to specific environment
  "query": {
    "bool": {
      "filter": [{ "term": { "service.environment": "production" } }]
    }
  }
}

Will the query specified by APM over the API and the query in the config file be merged? Or should we change the query to:

  "query": {
    "bool": {
      "filter": [
        { "term": { "processor.event": "transaction" } },
        { "exists": { "field": "transaction.duration.us" } }
        { "term": { "service.environment": "production" } }]
    }
  }

?

Copy link
Member

Choose a reason for hiding this comment

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

Btw. overall does the request look correct?

Copy link
Member

@jgowdyelastic jgowdyelastic Jul 1, 2020

Choose a reason for hiding this comment

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

the query will not get merged, it will overwrite the query in each datafeed in the module.

here’s a suggestion for the setup request.
i’ve moved the custom settings into the jobOverrides and added a job_tags property. l’m not sold on that name, so better suggestions are welcome.
i’ve also removed the groups override because it’s not needed, all APM modules already have this group set.

POST /api/ml/modules/setup/apm_transaction
{
  // tagging the job with service.environment
  "jobOverrides": {
    "custom_settings": {
      "job_tags": {
        "service.environment": "production"
       }
     }
  },
  // specifying the indicies to query
  "indexPatternName": "apm-*-transaction-*",
  // create job and start immediately
  "startDatafeed": true,
  // limit job to specific environment
  "query": {
    "bool": {
      "filter": [
        { "term": { "processor.event": "transaction" } },
        { "exists": { "field": "transaction.duration.us" } }
        { "term": { "service.environment": "production" } }]
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jgowdyelastic !

Is jobOverrides requried? I thought this would do:

POST /api/ml/modules/setup/apm_transaction
{
  "custom_settings": {
    "job_tags": {
      "service.environment": "production"
     }
   }
  //...
}

Copy link
Member

Choose a reason for hiding this comment

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

yes, with it you can override any part of the job.

Copy link
Member

@sorenlouv sorenlouv Jul 2, 2020

Choose a reason for hiding this comment

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

Okay, so do we then also need it for the other attributes like query?

{
  "jobOverrides": {
    "query": {
      "bool": {
        "filter": [
          { "term": { "processor.event": "transaction" } },
          { "exists": { "field": "transaction.duration.us" } },
          { "term": { "service.environment": "production" } }
        ]
      }
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

no, only for custom_settings.
jobOverrides and datafeedOverrides can be thought of as additional or advanced overrides for the job.
#42946

Copy link
Member

Choose a reason for hiding this comment

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

Okay - that sounds a bit inconsistent to me. Is the intention to align this? Is it documented somewhere which fields need to be wrapped in jobOverrides and which don't?

Copy link
Member

Choose a reason for hiding this comment

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

query and groups are the only overrides which could potentially be moved into jobOverrides' and 'datafeedOverrides. For the sake of backwards compatibility I'm happy for them to stay where there are, unless others agree that they are confusing and should be moved.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for all the help @blaklaybul

"type": "Transaction data",
"logoFile": "logo.json",
"defaultIndexPattern": "apm-*",
"defaultIndexPattern": "apm-*-transaction",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking that this only has the * wildcard in the middle of the pattern, and not at the end too, as we have apm-*-transaction-* in the example provided to the setup endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are two index patterns in the test data provided by @dgieselaar : apm-* and apm-*-transaction. I went with the latter since the module apm_transaction focuses only on transaction data. @dgieselaar will apm-*-transaction reliably exist? If not, @peteharverson are there any potential consequences of having a nonexistent index pattern here?

Copy link
Member

@dgieselaar dgieselaar Jul 1, 2020

Choose a reason for hiding this comment

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

@blaklaybul it's configurable, so no guarantees (for both apm-* and apm-*-transaction). I'm assuming we set this when we create the job (have to check but can't find the code right now).

Copy link
Contributor

Choose a reason for hiding this comment

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

@blaklaybul the defaultIndexPattern supplied in the module manifest.json is just used as a fallback by our module endpoints if no indexPatternName is supplied to the endpoint. So for the common use case, where the index pattern is supplied to the setup endpoint to create the jobs, this value from the manifest won't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ogupte
Copy link
Contributor

ogupte commented Jul 1, 2020

Is there an API within the ML plugin contract to do the job creation, or is only accessible on the REST endpoint POST /api/ml/modules/setup/apm_transaction?

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jul 1, 2020

@ogupte yes, setupModuleItems is available in modulesProvider shared in our setup contract.

@ogupte
Copy link
Contributor

ogupte commented Jul 2, 2020

In APM, we've hit a wall with the ML integration for 7.9 until this work is merged. I marked my PR (#70560) with the 'blocked' tag for now, while I paralleize on other integration areas. Here are the lines where the actual job creation code is: https://github.com/elastic/kibana/pull/70560/files#diff-e96da07e4982cbb34a6455843f012f49R84, if you could verify the jobOverrides looks correct on our end. Also are datafeedOverrides, start, and end values even necessary for this kind of job?

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested the new module from within the ML plugin using a small data set. All worked fine. Just left a couple of minor comments on the description text.

@blaklaybul blaklaybul requested a review from peteharverson July 2, 2020 12:38
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Changes to the ML module files LGTM

@blaklaybul
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

blaklaybul pushed a commit that referenced this pull request Jul 2, 2020
* updates apm integration job to work with service maps

* rename apm job in setup_module test

* modifies detector description

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants