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

engine: per try idle timeout #1805

Merged
merged 13 commits into from
Sep 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 18 additions & 1 deletion docs/root/api/starting_envoy.rst
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ Specify the rate at which Envoy Mobile should flush its queued stats.

Specifies the length of time a stream should wait without a headers or data event before timing out.
Defaults to 15 seconds.
See `the Envoy docs <https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-stream-idle-timeout>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why @buildbreaker updated. But still seems to generate correctly.

See `the Envoy docs <https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-stream-idle-timeout>`__
for further information.

**Example**::
Expand All @@ -185,6 +185,23 @@ for further information.
// Swift
builder.addStreamIdleTimeoutSeconds(5)

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
``addPerTryIdleTimeoutSeconds``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Specifies the length of time a retry (including the initial attempt) should wait without a headers
or data event before timing out. Defaults to 15 seconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this apply to trailers?

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, this should say "wait for bytes". That would be more comprehensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

@buildbreaker let me fix this in a subsequent PR so we don't have to run CI on this again.

See `the Envoy docs <https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto.html#config-route-v3-retrypolicy>`__
for further information.

**Example**::

// Kotlin
builder.addPerTryIdleTimeoutSeconds(5L)

// Swift
builder.addPerTryIdleTimeoutSeconds(5)

~~~~~~~~~~~~~~~~~
``addAppVersion``
~~~~~~~~~~~~~~~~~
Expand Down
1 change: 1 addition & 0 deletions library/cc/engine_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ std::string EngineBuilder::generateConfigStr() {
{"stats_domain", this->stats_domain_},
{"stats_flush_interval", fmt::format("{}s", this->stats_flush_seconds_)},
{"stream_idle_timeout", fmt::format("{}s", this->stream_idle_timeout_seconds_)},
{"per_try_idle_timeout", fmt::format("{}s", this->per_try_idle_timeout_seconds_)},
{"virtual_clusters", this->virtual_clusters_},
};

Expand Down
1 change: 1 addition & 0 deletions library/cc/engine_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class EngineBuilder {
std::string device_os_ = "unspecified";
std::string virtual_clusters_ = "[]";
int stream_idle_timeout_seconds_ = 15;
int per_try_idle_timeout_seconds_ = 15;

// TODO(crockeo): add after filter integration
// private var platformFilterChain = mutableListOf<EnvoyHTTPFilterFactory>()
Expand Down
2 changes: 2 additions & 0 deletions library/common/config/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const std::string config_header = R"(
- &statsd_host 127.0.0.1
- &statsd_port 8125
- &stream_idle_timeout 15s
- &per_try_idle_timeout 15s
- &virtual_clusters []

!ignore stats_defs:
Expand Down Expand Up @@ -228,6 +229,7 @@ const char* config_template = R"(
cluster_header: x-envoy-mobile-cluster
timeout: 0s
retry_policy:
per_try_idle_timeout: *per_try_idle_timeout
retry_back_off:
base_interval: 0.25s
max_interval: 60s
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public class EnvoyConfiguration {
public final List<EnvoyHTTPFilterFactory> httpPlatformFilterFactories;
public final Integer statsFlushSeconds;
public final Integer streamIdleTimeoutSeconds;
public final Integer perTryIdleTimeoutSeconds;
public final String appVersion;
public final String appId;
public final String virtualClusters;
Expand All @@ -50,6 +51,7 @@ public class EnvoyConfiguration {
* @param h2ConnectionKeepaliveTimeoutSeconds rate in seconds to timeout h2 pings.
* @param statsFlushSeconds interval at which to flush Envoy stats.
* @param streamIdleTimeoutSeconds idle timeout for HTTP streams.
* @param perTryIdleTimeoutSeconds per try idle timeout for HTTP streams.
* @param appVersion the App Version of the App using this Envoy Client.
* @param appId the App ID of the App using this Envoy Client.
* @param virtualClusters the JSON list of virtual cluster configs.
Expand All @@ -64,8 +66,9 @@ public EnvoyConfiguration(Boolean adminInterfaceEnabled, String grpcStatsDomain,
String dnsPreresolveHostnames,
int h2ConnectionKeepaliveIdleIntervalMilliseconds,
int h2ConnectionKeepaliveTimeoutSeconds, int statsFlushSeconds,
int streamIdleTimeoutSeconds, String appVersion, String appId,
String virtualClusters, List<EnvoyNativeFilterConfig> nativeFilterChain,
int streamIdleTimeoutSeconds, int perTryIdleTimeoutSeconds,
String appVersion, String appId, String virtualClusters,
List<EnvoyNativeFilterConfig> nativeFilterChain,
List<EnvoyHTTPFilterFactory> httpPlatformFilterFactories,
Map<String, EnvoyStringAccessor> stringAccessors) {
this.adminInterfaceEnabled = adminInterfaceEnabled;
Expand All @@ -82,6 +85,7 @@ public EnvoyConfiguration(Boolean adminInterfaceEnabled, String grpcStatsDomain,
this.h2ConnectionKeepaliveTimeoutSeconds = h2ConnectionKeepaliveTimeoutSeconds;
this.statsFlushSeconds = statsFlushSeconds;
this.streamIdleTimeoutSeconds = streamIdleTimeoutSeconds;
this.perTryIdleTimeoutSeconds = perTryIdleTimeoutSeconds;
this.appVersion = appVersion;
this.appId = appId;
this.virtualClusters = virtualClusters;
Expand Down Expand Up @@ -133,6 +137,7 @@ String resolveTemplate(final String templateYAML, final String platformFilterTem
.append(String.format("- &h2_connection_keepalive_timeout %ss\n",
h2ConnectionKeepaliveTimeoutSeconds))
.append(String.format("- &stream_idle_timeout %ss\n", streamIdleTimeoutSeconds))
.append(String.format("- &per_try_idle_timeout %ss\n", perTryIdleTimeoutSeconds))
.append(String.format("- &metadata { device_os: %s, app_version: %s, app_id: %s }\n",
"Android", appVersion, appId))
.append("- &virtual_clusters ")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class NativeCronetEngineBuilderImpl extends CronetEngineBuilderImpl {
private int mH2ConnectionKeepaliveTimeoutSeconds = 10;
private int mStatsFlushSeconds = 60;
private int mStreamIdleTimeoutSeconds = 15;
private int mPerTryIdleTimeoutSeconds = 15;
private String mAppVersion = "unspecified";
private String mAppId = "unspecified";
private String mVirtualClusters = "[]";
Expand Down Expand Up @@ -75,7 +76,7 @@ private EnvoyConfiguration createEnvoyConfiguration() {
mDnsRefreshSeconds, mDnsFailureRefreshSecondsBase, mDnsFailureRefreshSecondsMax,
mDnsQueryTimeoutSeconds, mDnsPreresolveHostnames,
mH2ConnectionKeepaliveIdleIntervalMilliseconds, mH2ConnectionKeepaliveTimeoutSeconds,
mStatsFlushSeconds, mStreamIdleTimeoutSeconds, mAppVersion, mAppId, mVirtualClusters,
mNativeFilterChain, mPlatformFilterChain, mStringAccessors);
mStatsFlushSeconds, mStreamIdleTimeoutSeconds, mPerTryIdleTimeoutSeconds, mAppVersion,
mAppId, mVirtualClusters, mNativeFilterChain, mPlatformFilterChain, mStringAccessors);
}
}
21 changes: 17 additions & 4 deletions library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ open class EngineBuilder(
private var h2ConnectionKeepaliveTimeoutSeconds = 10
private var statsFlushSeconds = 60
private var streamIdleTimeoutSeconds = 15
private var perTryIdleTimeoutSeconds = 15
private var appVersion = "unspecified"
private var appId = "unspecified"
private var virtualClusters = "[]"
Expand Down Expand Up @@ -199,6 +200,18 @@ open class EngineBuilder(
return this
}

/**
* Add a custom per try idle timeout for HTTP streams. Defaults to 15 seconds.
*
* @param perTryIdleTimeoutSeconds per try idle timeout for HTTP streams.
*
* @return this builder.
*/
fun addPerTryIdleTimeoutSeconds(perTryIdleTimeoutSeconds: Int): EngineBuilder {
this.perTryIdleTimeoutSeconds = perTryIdleTimeoutSeconds
return this
}

/**
* Add an HTTP filter factory used to create platform filters for streams sent by this client.
*
Expand Down Expand Up @@ -350,8 +363,8 @@ open class EngineBuilder(
dnsRefreshSeconds, dnsFailureRefreshSecondsBase, dnsFailureRefreshSecondsMax,
dnsQueryTimeoutSeconds, dnsPreresolveHostnames,
h2ConnectionKeepaliveIdleIntervalMilliseconds, h2ConnectionKeepaliveTimeoutSeconds,
statsFlushSeconds, streamIdleTimeoutSeconds, appVersion, appId,
virtualClusters, nativeFilterChain, platformFilterChain, stringAccessors
statsFlushSeconds, streamIdleTimeoutSeconds, perTryIdleTimeoutSeconds, appVersion,
appId, virtualClusters, nativeFilterChain, platformFilterChain, stringAccessors
),
configuration.yaml,
logLevel
Expand All @@ -365,8 +378,8 @@ open class EngineBuilder(
dnsRefreshSeconds, dnsFailureRefreshSecondsBase, dnsFailureRefreshSecondsMax,
dnsQueryTimeoutSeconds, dnsPreresolveHostnames,
h2ConnectionKeepaliveIdleIntervalMilliseconds, h2ConnectionKeepaliveTimeoutSeconds,
statsFlushSeconds, streamIdleTimeoutSeconds, appVersion, appId,
virtualClusters, nativeFilterChain, platformFilterChain, stringAccessors
statsFlushSeconds, streamIdleTimeoutSeconds, perTryIdleTimeoutSeconds, appVersion,
appId, virtualClusters, nativeFilterChain, platformFilterChain, stringAccessors
),
logLevel
)
Expand Down
4 changes: 4 additions & 0 deletions library/objective-c/EnvoyConfiguration.m
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ - (instancetype)initWithAdminInterfaceEnabled:(BOOL)adminInterfaceEnabled
h2ConnectionKeepaliveTimeoutSeconds:(UInt32)h2ConnectionKeepaliveTimeoutSeconds
statsFlushSeconds:(UInt32)statsFlushSeconds
streamIdleTimeoutSeconds:(UInt32)streamIdleTimeoutSeconds
perTryIdleTimeoutSeconds:(UInt32)perTryIdleTimeoutSeconds
appVersion:(NSString *)appVersion
appId:(NSString *)appId
virtualClusters:(NSString *)virtualClusters
Expand Down Expand Up @@ -47,6 +48,7 @@ - (instancetype)initWithAdminInterfaceEnabled:(BOOL)adminInterfaceEnabled
self.h2ConnectionKeepaliveTimeoutSeconds = h2ConnectionKeepaliveTimeoutSeconds;
self.statsFlushSeconds = statsFlushSeconds;
self.streamIdleTimeoutSeconds = streamIdleTimeoutSeconds;
self.perTryIdleTimeoutSeconds = perTryIdleTimeoutSeconds;
self.appVersion = appVersion;
self.appId = appId;
self.virtualClusters = virtualClusters;
Expand Down Expand Up @@ -122,6 +124,8 @@ - (nullable NSString *)resolveTemplate:(NSString *)templateYAML {
(unsigned long)self.h2ConnectionKeepaliveTimeoutSeconds];
[definitions
appendFormat:@"- &stream_idle_timeout %lus\n", (unsigned long)self.streamIdleTimeoutSeconds];
[definitions
appendFormat:@"- &per_try_idle_timeout %lus\n", (unsigned long)self.perTryIdleTimeoutSeconds];
[definitions appendFormat:@"- &metadata { device_os: %@, app_version: %@, app_id: %@ }\n", @"iOS",
self.appVersion, self.appId];
[definitions appendFormat:@"- &virtual_clusters %@\n", self.virtualClusters];
Expand Down
2 changes: 2 additions & 0 deletions library/objective-c/EnvoyEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ extern const int kEnvoyFilterResumeStatusResumeIteration;
@property (nonatomic, assign) UInt32 h2ConnectionKeepaliveTimeoutSeconds;
@property (nonatomic, assign) UInt32 statsFlushSeconds;
@property (nonatomic, assign) UInt32 streamIdleTimeoutSeconds;
@property (nonatomic, assign) UInt32 perTryIdleTimeoutSeconds;
@property (nonatomic, strong) NSString *appVersion;
@property (nonatomic, strong) NSString *appId;
@property (nonatomic, strong) NSString *virtualClusters;
Expand All @@ -330,6 +331,7 @@ extern const int kEnvoyFilterResumeStatusResumeIteration;
h2ConnectionKeepaliveTimeoutSeconds:(UInt32)h2ConnectionKeepaliveTimeoutSeconds
statsFlushSeconds:(UInt32)statsFlushSeconds
streamIdleTimeoutSeconds:(UInt32)streamIdleTimeoutSeconds
perTryIdleTimeoutSeconds:(UInt32)perTryIdleTimeoutSeconds
appVersion:(NSString *)appVersion
appId:(NSString *)appId
virtualClusters:(NSString *)virtualClusters
Expand Down
13 changes: 13 additions & 0 deletions library/swift/EngineBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ open class EngineBuilder: NSObject {
private var h2ConnectionKeepaliveTimeoutSeconds: UInt32 = 10
private var statsFlushSeconds: UInt32 = 60
private var streamIdleTimeoutSeconds: UInt32 = 15
private var perTryIdleTimeoutSeconds: UInt32 = 15
private var appVersion: String = "unspecified"
private var appId: String = "unspecified"
private var virtualClusters: String = "[]"
Expand Down Expand Up @@ -180,6 +181,17 @@ open class EngineBuilder: NSObject {
return self
}

/// Add a custom per try idle timeout for HTTP streams. Defaults to 15 seconds.
///
/// - parameter perTryIdleSeconds: Idle timeout for HTTP streams.
///
/// - returns: This builder.
@discardableResult
public func addPerTryIdleTimeoutSeconds(_ perTryIdleTimeoutSeconds: UInt32) -> Self {
self.perTryIdleTimeoutSeconds = perTryIdleTimeoutSeconds
return self
}

/// Add an HTTP platform filter factory used to construct filters for streams sent by this client.
///
/// - parameter name: Custom name to use for this filter factory. Useful for having
Expand Down Expand Up @@ -332,6 +344,7 @@ open class EngineBuilder: NSObject {
h2ConnectionKeepaliveTimeoutSeconds: self.h2ConnectionKeepaliveTimeoutSeconds,
statsFlushSeconds: self.statsFlushSeconds,
streamIdleTimeoutSeconds: self.streamIdleTimeoutSeconds,
perTryIdleTimeoutSeconds: self.perTryIdleTimeoutSeconds,
appVersion: self.appVersion,
appId: self.appId,
virtualClusters: self.virtualClusters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ class EnvoyConfigurationTest {
@Test
fun `resolving with default configuration resolves with values`() {
val envoyConfiguration = EnvoyConfiguration(
false, "stats.foo.com", null, 123, 234, 345, 456, 321, "[hostname]", 222, 333, 567, 678, "v1.2.3", "com.mydomain.myapp", "[test]",
listOf<EnvoyNativeFilterConfig>(EnvoyNativeFilterConfig("filter_name", "test_config")),
false, "stats.foo.com", null, 123, 234, 345, 456, 321, "[hostname]", 222, 333, 567, 678, 910, "v1.2.3", "com.mydomain.myapp", "[test]",
listOf(EnvoyNativeFilterConfig("filter_name", "test_config")),
emptyList(), emptyMap()
)

Expand Down Expand Up @@ -62,6 +62,10 @@ class EnvoyConfigurationTest {
assertThat(resolvedTemplate).contains("&stats_domain stats.foo.com")
assertThat(resolvedTemplate).contains("&stats_flush_interval 567s")

// Idle timeouts
assertThat(resolvedTemplate).contains("&stream_idle_timeout 678s")
assertThat(resolvedTemplate).contains("&per_try_idle_timeout 910s")

// Filters
assertThat(resolvedTemplate).contains("filter_name")
assertThat(resolvedTemplate).contains("test_config")
Expand All @@ -70,7 +74,7 @@ class EnvoyConfigurationTest {
@Test
fun `resolve templates with invalid templates will throw on build`() {
val envoyConfiguration = EnvoyConfiguration(
false, "stats.foo.com", null, 123, 234, 345, 456, 321, "[hostname]", 123, 123, 567, 678, "v1.2.3", "com.mydomain.myapp", "[test]",
false, "stats.foo.com", null, 123, 234, 345, 456, 321, "[hostname]", 123, 123, 567, 678, 910, "v1.2.3", "com.mydomain.myapp", "[test]",
emptyList(), emptyList(), emptyMap()
)

Expand All @@ -85,7 +89,7 @@ class EnvoyConfigurationTest {
@Test
fun `cannot configure both statsD and gRPC stat sink`() {
val envoyConfiguration = EnvoyConfiguration(
false, "stats.foo.com", 5050, 123, 234, 345, 456, 321, "[hostname]", 123, 123, 567, 678, "v1.2.3", "com.mydomain.myapp", "[test]",
false, "stats.foo.com", 5050, 123, 234, 345, 456, 321, "[hostname]", 123, 123, 567, 678, 910, "v1.2.3", "com.mydomain.myapp", "[test]",
emptyList(), emptyList(), emptyMap()
)

Expand Down
10 changes: 10 additions & 0 deletions test/kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,16 @@ class EngineBuilderTest {
assertThat(engine.envoyConfiguration!!.streamIdleTimeoutSeconds).isEqualTo(1234)
}

@Test
fun `specifying per try idle timeout overrides default`() {
engineBuilder = EngineBuilder(Standard())
engineBuilder.addEngineType { envoyEngine }
engineBuilder.addPerTryIdleTimeoutSeconds(5678)

val engine = engineBuilder.build() as EngineImpl
assertThat(engine.envoyConfiguration!!.perTryIdleTimeoutSeconds).isEqualTo(5678)
}

@Test
fun `specifying app version overrides default`() {
engineBuilder = EngineBuilder(Standard())
Expand Down
17 changes: 17 additions & 0 deletions test/swift/EngineBuilderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,20 @@ final class EngineBuilderTests: XCTestCase {
self.waitForExpectations(timeout: 0.01)
}

func testAddingPerTryIdleTimeoutSecondsAddsToConfigurationWhenRunningEnvoy() {
let expectation = self.expectation(description: "Run called with expected data")
MockEnvoyEngine.onRunWithConfig = { config, _ in
XCTAssertEqual(21, config.perTryIdleTimeoutSeconds)
expectation.fulfill()
}

_ = EngineBuilder()
.addEngineType(MockEnvoyEngine.self)
.addPerTryIdleTimeoutSeconds(21)
.build()
self.waitForExpectations(timeout: 0.01)
}

func testAddingAppVersionAddsToConfigurationWhenRunningEnvoy() {
let expectation = self.expectation(description: "Run called with expected data")
MockEnvoyEngine.onRunWithConfig = { config, _ in
Expand Down Expand Up @@ -306,6 +320,7 @@ final class EngineBuilderTests: XCTestCase {
h2ConnectionKeepaliveTimeoutSeconds: 333,
statsFlushSeconds: 600,
streamIdleTimeoutSeconds: 700,
perTryIdleTimeoutSeconds: 777,
appVersion: "v1.2.3",
appId: "com.envoymobile.ios",
virtualClusters: "[test]",
Expand All @@ -331,6 +346,7 @@ final class EngineBuilderTests: XCTestCase {
XCTAssertTrue(resolvedYAML.contains("&h2_connection_keepalive_timeout 333s"))

XCTAssertTrue(resolvedYAML.contains("&stream_idle_timeout 700s"))
XCTAssertTrue(resolvedYAML.contains("&per_try_idle_timeout 777s"))

XCTAssertFalse(resolvedYAML.contains("admin: *admin_interface"))

Expand Down Expand Up @@ -365,6 +381,7 @@ final class EngineBuilderTests: XCTestCase {
h2ConnectionKeepaliveTimeoutSeconds: 333,
statsFlushSeconds: 600,
streamIdleTimeoutSeconds: 700,
perTryIdleTimeoutSeconds: 700,
appVersion: "v1.2.3",
appId: "com.envoymobile.ios",
virtualClusters: "[test]",
Expand Down