Skip to content

[Monitoring] Make Exporters Async#35765

Merged
pickypg merged 8 commits intoelastic:masterfrom
pickypg:fix/use-async-exporters
Nov 27, 2018
Merged

[Monitoring] Make Exporters Async#35765
pickypg merged 8 commits intoelastic:masterfrom
pickypg:fix/use-async-exporters

Conversation

@pickypg
Copy link
Copy Markdown
Member

@pickypg pickypg commented Nov 21, 2018 •
edited
Loading

This changes the exporter code -- most notably the http exporter -- to use async operations throughout the resource management and bulk initialization code (the bulk indexing of monitoring documents was already async).

As part of this change, this does change one semi-core aspect of the HttpResource class in that it will no longer block all concurrent calls until the first call completes with HttpResource::checkAndPublishIfDirty. Now, any parallel attempts to check the resources will be skipped until the first call completes (success or failure). While this is a technical change, it has very little practical impact because the existing behavior was either quick success (then every blocked request processed) or each request timed out and failed anyway, thus being effectively skipped (and a burden on the system).

Closes #35743

@pickypg pickypg requested a review from martijnvg November 21, 2018 03:03
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Nov 21, 2018

Pinging @elastic/es-core-features

@pickypg
Copy link
Copy Markdown
Member Author

pickypg commented Nov 21, 2018

Note: I haven't updated any of the tests yet. I wanted to validate my approach as the amount of code changed was larger than I had initially expected. I'm fairly sure that the direction is the intended one, but before I go down the path of changing all of the tests, I've got it working here async this way.

@pickypg
Copy link
Copy Markdown
Member Author

pickypg commented Nov 21, 2018

/cc @elastic/kibana-monitoring

martijnvg
martijnvg reviewed Nov 21, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @pickypg! This PR is in the right direction. I left some small comments and a bigger comment about the fact that resolveBulk(...) method in LocalExporter also needs to be made async.

@pickypg pickypg mentioned this pull request Nov 21, 2018
Closed
@pickypg pickypg force-pushed the fix/use-async-exporters branch from ed7dc58 to 32305dd Compare November 26, 2018 21:05
@pickypg
Copy link
Copy Markdown
Member Author

pickypg commented Nov 26, 2018

@martijnvg This one is ready for a full review now that the tests have been updated.

martijnvg
martijnvg approved these changes Nov 27, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Left one small comment - LGTM.

@pickypg
[Monitoring] Make Exporters Async
dc71a8c 
This changes the exporter code -- most notably the `http` exporter --
to use async operations throughout the resource management and bulk
initialization code (the bulk indexing of monitoring documents was
already async).

As part of this change, this does change one semi-core aspect of the
`HttpResource` class in that it will no longer block all concurrent
calls until the first call completes with
`HttpResource#checkAndPublishIfDirty`. Now, the any incoming attempts
to check the resources will be skipped until the first call completes
(success or failure). While this is a technical change, it has very
little practical impact because the existing behavior was either
success, then every processed quickly or each request timed out and
failed anyway, thus being effectively skipped.
@pickypg
Address comments
a7af9ad
@pickypg
Update tests for async operations
4cef060
@pickypg
fix copyright
e72af07
@pickypg
remove unused export
6c9d72f
@pickypg
Assert latch.await returns true and always return from listener
3cd1182
@pickypg
remove debug logging
dc2a9d6
@pickypg
make test method package protected
de60734
@pickypg pickypg force-pushed the fix/use-async-exporters branch from 581963c to de60734 Compare November 27, 2018 17:52
@pickypg
Copy link
Copy Markdown
Member Author

pickypg commented Nov 27, 2018

Rebased for cluster upgrade test failure.

@pickypg pickypg merged commit 3337fa7 into elastic:master Nov 27, 2018
@pickypg pickypg deleted the fix/use-async-exporters branch November 27, 2018 19:28
pickypg added a commit that referenced this pull request Nov 27, 2018
@pickypg
[Monitoring] Make Exporters Async (#35765)
9ffc492 
This changes the exporter code -- most notably the `http` exporter --
to use async operations throughout the resource management and bulk
initialization code (the bulk indexing of monitoring documents was
already async).

As part of this change, this does change one semi-core aspect of the
`HttpResource` class in that it will no longer block all concurrent calls
until the first call completes with
`HttpResource::checkAndPublishIfDirty`.
Now, any parallel attempts to check the resources will be skipped until
the first call completes (success or failure). While this is a technical
change, it has very little practical impact because the existing behavior
was either quick success (then every blocked request processed) or
each request timed out and failed anyway, thus being effectively
skipped (and a burden on the system).
@pickypg
Copy link
Copy Markdown
Member Author

pickypg commented Nov 27, 2018 •
edited
Loading

6.x/6.6: 9ffc492

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Nov 28, 2018
@jasontedor
Merge branch 'master' into xpack-license
6037c8b 
* master:
  DOCS Audit event attributes in new format (elastic#35510)
  Scripting: Actually add joda time back to whitelist (elastic#35965)
  [DOCS] fix HLRC ILM doc misreferenced tag
  Add realm information for Authenticate API (elastic#35648)
  [ILM] add HLRC docs to remove-policy-from-index (elastic#35759)
  [Rollup] Update serialization version after backport
  [Rollup] Add more diagnostic stats to job (elastic#35471)
  Build: Fix gradle build for Mac OS (elastic#35968)
  Adds deprecation logging to ScriptDocValues#getValues. (elastic#34279)
  [Monitoring] Make Exporters Async (elastic#35765)
  [ILM] reduce time restriction on IndexLifecycleExplainResponse (elastic#35954)
  Remove use of AbstractComponent in xpack (elastic#35394)
  Deprecate types in search and multi search templates. (elastic#35669)
  Remove fromXContent from IndexUpgradeInfoResponse (elastic#35934)
@pickypg pickypg mentioned this pull request Dec 13, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martijnvg martijnvg martijnvg approved these changes

Assignees

No one assigned

Labels

:Core/Infra/Monitoring DEPRECATED, DO NOT USE >enhancement v6.6.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pickypg @elasticmachine @martijnvg @colings86