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 other time accounting in HotThreads #79392

Merged
merged 17 commits into from
Oct 19, 2021
Merged

Add other time accounting in HotThreads #79392

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5d8c9ca
Add other time accounting in HotThreads
Oct 14, 2021
a434a16
Improve sampling of wait/blocked.
Oct 15, 2021
75d7e85
Small refactor.
Oct 15, 2021
d6e5947
Refactor.
Oct 15, 2021
d9d746c
Add sort everywhere.
Oct 15, 2021
16520a2
Make monitoring part of bootstrap
Oct 18, 2021
22c3be8
Update tests and remove security setting.
Oct 18, 2021
423ad61
Make code clearer.
Oct 18, 2021
ec7848e
Fix style and tests.
Oct 18, 2021
5dd7579
Implement comments from Ryan.
Oct 18, 2021
1417916
Undo edit.
Oct 18, 2021
4bc05a0
Merge branch 'master' into feature/hot_threads_other
Oct 18, 2021
3dcec18
Merge branch 'master' into feature/hot_threads_other
elasticmachine Oct 19, 2021
9eb0804
Fix test
Oct 19, 2021
f06b946
Change version from static to straight constant.
Oct 19, 2021
25ab3d7
Merge branch 'feature/hot_threads_other' of github.com:grcevski/elast…
Oct 19, 2021
c1c57d6
Apply changes as per review.
Oct 19, 2021
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
19 changes: 19 additions & 0 deletions ...n/java/org/elasticsearch/action/admin/cluster/node/hotthreads/NodesHotThreadsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

package org.elasticsearch.action.admin.cluster.node.hotthreads;

import org.elasticsearch.Version;
import org.elasticsearch.action.support.nodes.BaseNodesRequest;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.io.stream.StreamInput;
Expand All @@ -20,8 +21,11 @@

public class NodesHotThreadsRequest extends BaseNodesRequest<NodesHotThreadsRequest> {

private static final Version SORT_SUPPORTED_VERSION = Version.V_8_0_0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once we land this in 7_16_0, I'll change 8.0.0 to reflect that.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to see these versions directly in the streaminput ctor and writeTo methods. As a constant, it looks as if the value could just be changed, but it can't.

Copy link
Member

Choose a reason for hiding this comment

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

I also have a person preference for setting the version to the intended backport version from the start, but what you described is acceptable. Thanks for noting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Changed.


int threads = 3;
HotThreads.ReportType type = HotThreads.ReportType.CPU;
Copy link
Member

Choose a reason for hiding this comment

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

These should all be final, I'm surprised they are not. We should have a ctor to take all the members. That can be a followup, though (doesn't have to make 7.16 since this is already the state of the class).

HotThreads.SortOrder sortOrder = HotThreads.SortOrder.TOTAL;
TimeValue interval = new TimeValue(500, TimeUnit.MILLISECONDS);
int snapshots = 10;
boolean ignoreIdleThreads = true;
Expand All @@ -34,6 +38,9 @@ public NodesHotThreadsRequest(StreamInput in) throws IOException {
type = HotThreads.ReportType.of(in.readString());
interval = in.readTimeValue();
snapshots = in.readInt();
if (in.getVersion().onOrAfter(SORT_SUPPORTED_VERSION)) {
sortOrder = HotThreads.SortOrder.of(in.readString());
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice (as a followup) to look at changing read/write enum to use the value, instead of ordinal. As it is, the enum methods for stream input/output are brittle since they rely on the enum definition not changing.

}
}

/**
Expand Down Expand Up @@ -78,6 +85,15 @@ public HotThreads.ReportType type() {
return this.type;
}

public NodesHotThreadsRequest sortOrder(HotThreads.SortOrder sortOrder) {
this.sortOrder = sortOrder;
return this;
}

public HotThreads.SortOrder sortOrder() {
return this.sortOrder;
}

public NodesHotThreadsRequest interval(TimeValue interval) {
this.interval = interval;
return this;
Expand All @@ -104,5 +120,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(type.getTypeValue());
out.writeTimeValue(interval);
out.writeInt(snapshots);
if (out.getVersion().onOrAfter(SORT_SUPPORTED_VERSION)) {
out.writeString(sortOrder.getOrderValue());
}
}
}
5 changes: 5 additions & 0 deletions ...org/elasticsearch/action/admin/cluster/node/hotthreads/NodesHotThreadsRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ public NodesHotThreadsRequestBuilder setType(HotThreads.ReportType type) {
return this;
}

public NodesHotThreadsRequestBuilder setSortOrder(HotThreads.SortOrder sortOrder) {
request.sortOrder(sortOrder);
return this;
}

public NodesHotThreadsRequestBuilder setInterval(TimeValue interval) {
request.interval(interval);
return this;
Expand Down
1 change: 1 addition & 0 deletions ...rg/elasticsearch/action/admin/cluster/node/hotthreads/TransportNodesHotThreadsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ protected NodeHotThreads nodeOperation(NodeRequest request, Task task) {
HotThreads hotThreads = new HotThreads()
.busiestThreads(request.request.threads)
.type(request.request.type)
.sortOrder(request.request.sortOrder)
.interval(request.request.interval)
.threadElementsSnapshotCount(request.request.snapshots)
.ignoreIdleThreads(request.request.ignoreIdleThreads);
Expand Down
18 changes: 18 additions & 0 deletions server/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.io.IOException;
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.lang.management.ManagementFactory;
import java.net.URISyntaxException;
import java.nio.file.Path;
import java.security.NoSuchAlgorithmException;
Expand Down Expand Up @@ -158,6 +159,20 @@ static void initializeProbes() {
JvmInfo.jvmInfo();
}

static void initializeRuntimeMonitoring() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling thread contention monitoring on-demand, as we used to do it in HotThreads, is not precise enough to properly calculate the wait/blocked time. If we enable the support on demand, we need to introduce an artificial pause before we attempt to capture timings, which would make the API unnecessarily slow.

I ran various micro benchmarks and proved that enabling lock contention monitoring by default, doesn't impact the performance of either uncontended or heavily contended use case.

// We'll let the JVM boot without this functionality, however certain APIs like HotThreads will not
// function and report an error.
if (ManagementFactory.getThreadMXBean().isThreadContentionMonitoringSupported() == false) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in a static for hot threads still? Perhaps a static method could be called in initializeProbes? I think we should strive to keep Bootstrap as small as possible, without feature specific logic like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll change this.

LogManager.getLogger(Bootstrap.class).info("Thread wait/blocked time accounting not supported.");
} else {
try {
ManagementFactory.getThreadMXBean().setThreadContentionMonitoringEnabled(true);
} catch (UnsupportedOperationException monitoringUnavailable) {
LogManager.getLogger(Bootstrap.class).warn("Thread wait/blocked time accounting cannot be enabled.");
}
}
}

private void setup(boolean addShutdownHook, Environment environment) throws BootstrapException {
Settings settings = environment.settings();

Expand All @@ -176,6 +191,9 @@ private void setup(boolean addShutdownHook, Environment environment) throws Boot
// initialize probes before the security manager is installed
initializeProbes();

// initialize monitoring in the JVM runtime, helps with thread wait/blocked time accounting
initializeRuntimeMonitoring();

if (addShutdownHook) {
Runtime.getRuntime().addShutdownHook(new Thread() {
@Override
Expand Down
Loading