-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Add connection failure metrics in RemoteConnectionStrategy #137406
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
Changes from 10 commits
345acad
3acd2df
fdd31bd
ababa08
4f6263d
97bc022
96114ac
10cdd3b
af436be
5d6d9fd
d06dfbd
5a9fc0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |
| import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
| import org.elasticsearch.core.Strings; | ||
| import org.elasticsearch.core.TimeValue; | ||
| import org.elasticsearch.telemetry.InstrumentType; | ||
| import org.elasticsearch.telemetry.RecordingMeterRegistry; | ||
| import org.elasticsearch.test.ESTestCase; | ||
| import org.elasticsearch.test.EnumSerializationTestUtils; | ||
| import org.elasticsearch.test.MockLog; | ||
|
|
@@ -28,12 +30,15 @@ | |
| import org.elasticsearch.threadpool.TestThreadPool; | ||
| import org.elasticsearch.threadpool.ThreadPool; | ||
|
|
||
| import java.util.Set; | ||
|
|
||
| import static org.elasticsearch.test.MockLog.assertThatLogger; | ||
| import static org.elasticsearch.transport.RemoteClusterSettings.ProxyConnectionStrategySettings.PROXY_ADDRESS; | ||
| import static org.elasticsearch.transport.RemoteClusterSettings.REMOTE_CONNECTION_MODE; | ||
| import static org.elasticsearch.transport.RemoteClusterSettings.SniffConnectionStrategySettings.REMOTE_CLUSTER_SEEDS; | ||
| import static org.elasticsearch.transport.RemoteClusterSettings.toConfig; | ||
| import static org.elasticsearch.transport.RemoteConnectionStrategy.buildConnectionProfile; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class RemoteConnectionStrategyTests extends ESTestCase { | ||
|
|
@@ -194,7 +199,7 @@ public void testConnectionStrategySerialization() { | |
| value = "org.elasticsearch.transport.RemoteConnectionStrategyTests.FakeConnectionStrategy:DEBUG", | ||
| reason = "logging verification" | ||
| ) | ||
| public void testConnectionAttemptLogging() { | ||
| public void testConnectionAttemptMetricsAndLogging() { | ||
| final var originProjectId = randomUniqueProjectId(); | ||
| final var linkedProjectId = randomUniqueProjectId(); | ||
| final var alias = randomAlphanumericOfLength(10); | ||
|
|
@@ -208,16 +213,21 @@ public void testConnectionAttemptLogging() { | |
| new ClusterConnectionManager(TestProfiles.LIGHT_PROFILE, mock(Transport.class), threadContext) | ||
| ) | ||
| ) { | ||
| assert transportService.getTelemetryProvider() != null; | ||
| final var meterRegistry = transportService.getTelemetryProvider().getMeterRegistry(); | ||
| assert meterRegistry instanceof RecordingMeterRegistry; | ||
| final var metricRecorder = ((RecordingMeterRegistry) meterRegistry).getRecorder(); | ||
|
Comment on lines
+216
to
+219
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to my other comments about static fields. If we create two |
||
|
|
||
| for (boolean shouldConnectFail : new boolean[] { true, false }) { | ||
| for (boolean isIntialConnectAttempt : new boolean[] { true, false }) { | ||
| for (boolean isInitialConnectAttempt : new boolean[] { true, false }) { | ||
| final var strategy = new FakeConnectionStrategy( | ||
| originProjectId, | ||
| linkedProjectId, | ||
| alias, | ||
| transportService, | ||
| connectionManager | ||
| ); | ||
| if (isIntialConnectAttempt == false) { | ||
| if (isInitialConnectAttempt == false) { | ||
| waitForConnect(strategy); | ||
| } | ||
| strategy.setShouldConnectFail(shouldConnectFail); | ||
|
|
@@ -228,7 +238,7 @@ public void testConnectionAttemptLogging() { | |
| shouldConnectFail ? "failed to connect" : "successfully connected", | ||
| linkedProjectId, | ||
| alias, | ||
| isIntialConnectAttempt ? "the initial connection" : "a reconnection" | ||
| isInitialConnectAttempt ? "the initial connection" : "a reconnection" | ||
| ); | ||
| assertThatLogger(() -> { | ||
| if (shouldConnectFail) { | ||
|
|
@@ -243,12 +253,30 @@ public void testConnectionAttemptLogging() { | |
| + expectedLogLevel | ||
| + " after a " | ||
| + (shouldConnectFail ? "failed" : "successful") | ||
| + (isIntialConnectAttempt ? " initial connection attempt" : " reconnection attempt"), | ||
| + (isInitialConnectAttempt ? " initial connection attempt" : " reconnection attempt"), | ||
| strategy.getClass().getCanonicalName(), | ||
| expectedLogLevel, | ||
| expectedLogMessage | ||
| ) | ||
| ); | ||
| if (shouldConnectFail) { | ||
| metricRecorder.collect(); | ||
| final var counterName = RemoteClusterService.CONNECTION_ATTEMPT_FAILURES_COUNTER_NAME; | ||
| final var measurements = metricRecorder.getMeasurements(InstrumentType.LONG_COUNTER, counterName); | ||
| assertFalse(measurements.isEmpty()); | ||
| final var measurement = measurements.getLast(); | ||
| assertThat(measurement.getLong(), equalTo(1L)); | ||
| final var attributes = measurement.attributes(); | ||
| final var keySet = Set.of("linked_project_id", "linked_project_alias", "attempt", "strategy"); | ||
| final var expectedAttemptType = isInitialConnectAttempt | ||
| ? RemoteConnectionStrategy.ConnectionAttempt.initial | ||
| : RemoteConnectionStrategy.ConnectionAttempt.reconnect; | ||
| assertThat(attributes.keySet(), equalTo(keySet)); | ||
| assertThat(attributes.get("linked_project_id"), equalTo(linkedProjectId.toString())); | ||
| assertThat(attributes.get("linked_project_alias"), equalTo(alias)); | ||
| assertThat(attributes.get("attempt"), equalTo(expectedAttemptType.toString())); | ||
| assertThat(attributes.get("strategy"), equalTo(strategy.strategyType().toString())); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these null checks necessary? I assume they are only useful for tests? Does
TransportServicenot return aNOOPtelemetryProvider if a real one is not configured?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to eliminate most of the null checks here, the remaining one I'll address in a followup PR with some additional refactoring in RemoveClusterService tests.