stats: introduce stats client #992

jingwei99 · 2020-07-30T23:11:08Z

Description: Introduces a StatsClient capable of giving out Counters that, when incremented, emit stats from Envoy's internal stats engine. Also refactors the top-level platform Engine interface to make both the StreamClient and the StatsClient available.
Risk Level: Low
Testing: Pending

Co-authored-by: Mike Schore mike.schore@gmail.com

library/common/engine.cc

rebello95

Overall I like this approach! One thing I'm wondering about is if we can make elements more intuitive/user friendly in the public interface. Would it make sense to have the platform expose a strongly typed interface/type for that rather than having the user provide an array of strings?
Documentation on the expected usage would also be great.

goaway · 2020-08-03T11:16:50Z

@rebello95 I like the idea of having Element be a strong type. There are actually invalid character strings that one could pass, and we can prevent that by having Element have a conditional initializer. This would allow error discovery to happen more or less statically.

Co-authored-by: Jingwei <57155915+jingwei99@users.noreply.github.com> Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

examples/java/hello_world/MainActivity.java

Signed-off-by: Mike Schore <mike.schore@gmail.com>

library/kotlin/src/io/envoyproxy/envoymobile/BUILD

library/kotlin/src/io/envoyproxy/envoymobile/Counter.kt

library/kotlin/src/io/envoyproxy/envoymobile/Element.kt

library/kotlin/src/io/envoyproxy/envoymobile/Engine.kt

library/swift/src/StatsClient.swift

library/swift/src/StatsClientImpl.swift

library/swift/src/mocks/MockEnvoyEngine.swift

library/swift/src/StreamClientImpl.swift

rebello95 · 2020-08-06T20:23:06Z

library/kotlin/src/io/envoyproxy/envoymobile/StatsClientImpl.kt

+) : StatsClient {
+
+  override fun getCounter(vararg elements: Element): Counter {
+    return Counter(WeakReference(engine), elements.asList())


iOS has a strong ref below, we should keep them consistent (I think a strong ref is what you want here)

I believe the weak reference is intended. We would not want to have stats spread through the application to necessarily hold the engine alive, right?

Ah I see, this is passing a weak reference to the Counter, whereas on iOS the Counter itself makes the reference weak. Would it make sense to move WeakReference(engine) into the counter file's constructor to make it clear that it doesn't retain the engine?

Separately, I'm wondering if it makes sense to allow counters to retain the engine. Do we want the engine to stick around as long as a consumer is holding on to a counter ref?

Would it make sense to move WeakReference(engine) into the counter file's constructor to make it clear that it doesn't retain the engine?

yea

Do we want the engine to stick around as long as a consumer is holding on to a counter ref?

probably not, counters could be referenced by feature modules of the platform(iOS/android) code, and we probably don't want the feature modules to (indirectly) hold a strong ref to engine

My intuition right now is that it seems natural for engine clients to strongly retain the engine, but perhaps less so for the artifacts they produce. This has more to do with users of the library being able to implicitly manage the lifecycle of the engine than anything else. I should say that I'm not strongly convinced this is right, but it seems reasonable for now for experimentation.

Was there a resolution here? Looks like the weak ref is still being passed from this line

junr03

Some comments, and a big question.

junr03 · 2020-08-06T20:40:30Z

library/common/config_template.cc

@@ -204,6 +204,9 @@ stats_flush_interval: {{ stats_flush_interval_seconds }}s
        - safe_regex:
            google_re2: {}
            regex: '^http.dispatcher.*'
+        - safe_regex:
+            google_re2: {}
+            regex: '^client.*'


I'd prefer having a more restrictive rule so that people have to think before their application level stats get emitted.

what rules do you have in mind?
right now all the elements used for the stats are checked against the regex: ^[A-Za-z_]+\$

Talked with @junr03, long-term it might make sense to make people specify this in configuration, so that some thought goes into the stats they emit. For now, as a new, experimental interface, this is probably good enough to move forward.

library/common/engine.cc

library/common/engine.h

library/common/engine.cc

junr03 · 2020-08-06T20:57:17Z

library/kotlin/src/io/envoyproxy/envoymobile/StatsClientImpl.kt

+) : StatsClient {
+
+  override fun getCounter(vararg elements: Element): Counter {
+    return Counter(WeakReference(engine), elements.asList())


I believe the weak reference is intended. We would not want to have stats spread through the application to necessarily hold the engine alive, right?

junr03 · 2020-08-06T21:01:13Z

library/common/engine.cc

@@ -93,6 +93,19 @@ Engine::~Engine() {
  main_thread_.join();
 }

+void Engine::recordCounter(std::string elements, uint64_t count) {


Can we cover this in tests?

Adding a ticket to cover testing generally at this layer, will link here.

@goaway don't forget to add this ticket

Created (but it's internal).

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

junr03

My big question about performance was answered, and a disclaimer was added. @goaway and @jingwei99 will follow up with @jmarantz to discuss the dynamic vs static stats path.

1 missing issue. Otherwise, LGTM.

junr03 · 2020-08-10T21:48:34Z

library/common/engine.cc

@@ -93,6 +93,19 @@ Engine::~Engine() {
  main_thread_.join();
 }

+void Engine::recordCounter(std::string elements, uint64_t count) {


@goaway don't forget to add this ticket

jmarantz · 2020-08-10T21:59:07Z

library/common/engine.cc

+void Engine::recordCounter(std::string elements, uint64_t count) {
+  if (server_) {
+    server_->dispatcher().post([this, elements, count]() -> void {
+      static const std::string client = "client";


No non-pod statics (static init fiasco). Moreover what you really want here since this string is known at compile-time is to save a StatName for "client" using either a StatNamePool or StatNameManagedStorage. You want to do that in a context object that is created once at process startup and then re-used. Then you have no thread contention issues.

rebello95

Looks good overall, but some comments.

We seem to be missing Kotlin stats client tests
Can we also update documentation / tutorials which are now invalid due to the engine interface changes?
Would also be good to add an issue for documenting the stats interface and linking that in the PR description

rebello95 · 2020-08-10T22:48:42Z

library/kotlin/src/io/envoyproxy/envoymobile/BUILD

+        "StatsClient.kt",
+        "StatsClientImpl.kt",


Shouldn't these be in stats/ which is captured below with globbing?

I think these ideally live here, basically in the same level as StreamClient, as the interface Engine.kt exposes a StatsClient and a StreamClient.

rebello95 · 2020-08-10T22:50:11Z

library/kotlin/src/io/envoyproxy/envoymobile/StatsClientImpl.kt

+) : StatsClient {
+
+  override fun getCounter(vararg elements: Element): Counter {
+    return Counter(WeakReference(engine), elements.asList())


Was there a resolution here? Looks like the weak ref is still being passed from this line

rebello95 · 2020-08-10T22:51:26Z

library/kotlin/src/io/envoyproxy/envoymobile/stats/Element.kt

+ * Element values must conform to the regex /^[A-Za-z_]+$/.
+ */
+class Element(val element: String) {
+  init {
+    if (!Pattern.compile("^[A-Za-z_]+\$").matcher(element).matches()) {
+      throw IllegalArgumentException(
+        "Element values must conform to the regex /^[A-Za-z_]+$/"


Regex is hardcoded 3 places here. Can we use a constant instead so we don't accidentally fail to change one of the usages in the future?

rebello95 · 2020-08-10T22:51:48Z

library/swift/src/BUILD

+        "StatsClient.swift",
+        "StatsClientImpl.swift",


Same here regarding living in stats/

same as above, StatsClient and its implementation ideally live at the same level as StreamClient given the Engine.swift interface.

rebello95 · 2020-08-10T22:52:20Z

library/swift/src/StatsClient.swift

+  /// - parameter elements: Elements to identify a counter
+  /// - returns: A Counter based on the joined elements.


Suggested change

/// - parameter elements: Elements to identify a counter

/// - returns: A Counter based on the joined elements.

/// - parameter elements: Elements to identify a counter

///

/// - returns: A Counter based on the joined elements.

rebello95 · 2020-08-10T22:55:33Z

library/swift/test/StatsClientImplTests.swift

+    MockEnvoyEngine.onRecordCounter = nil
+  }
+
+  func testCounterDelegatesToEngine() throws {


Suggested change

func testCounterDelegatesToEngine() throws {

func testCounterDelegatesToEngine() {

rebello95 · 2020-08-10T22:55:44Z

library/swift/test/StatsClientImplTests.swift

+    XCTAssertEqual(actualCount, 1)
+  }
+
+  func testCounterDelegatesToEngineWithCount() throws {


Suggested change

func testCounterDelegatesToEngineWithCount() throws {

func testCounterDelegatesToEngineWithCount() {

rebello95 · 2020-08-10T22:56:03Z

library/swift/test/StatsClientImplTests.swift

+    XCTAssertEqual(actualCount, 5)
+  }
+
+  func testCounterWeaklyHoldsEngine() throws {


Suggested change

func testCounterWeaklyHoldsEngine() throws {

func testCounterWeaklyHoldsEngine() {

rebello95 · 2020-08-10T22:56:36Z

library/swift/test/StatsClientImplTests.swift

+import Foundation
+import XCTest
+
+final class StatsClientImplTests: XCTestCase {


Can we add these tests for Kotlin too?

rebello95 · 2020-08-10T22:56:56Z

library/swift/test/BUILD

+    name = "engine_builder_tests",
    srcs = [
-        "StreamClientBuilderTests.swift",
+        "EngineBuilderTests.swift",
+    ],
+    deps = [
+        "//library/objective-c:envoy_engine_objc_lib",
+    ],
+)
+
+envoy_mobile_swift_test(


nit: can we alphabetize these test cases?

thinking out loud: prolly worth adding a lint/pre-commit check for this in the long run

jingwei99 · 2020-08-11T00:51:21Z

@rebello95 : for this comment, yea i missed it in my previous commit, and have a fix locally, planned to push along with other suggested changes

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

jingwei99 · 2020-08-11T16:58:15Z

library/kotlin/test/io/envoyproxy/envoymobile/StatsClientImplTest.kt

+    assertThat(elementsCaptor.getValue()).isEqualTo("test.stat")
+    assertThat(countCaptor.getValue()).isEqualTo(5)
+  }
+}


tried to emulate StatsClientImplTests.testCounterWeaklyHoldsEngine, but I wasn't able to get the EnvoyEngine instance garbage collected - as calling System.gc() doesn't guarantee gc.

rebello95 · 2020-08-11T17:52:04Z

@jingwei99 @goaway can we update the docs to reflect the interface changes from this PR?

jingwei99 · 2020-08-11T17:58:22Z

@jingwei99 @goaway can we update the docs to reflect the interface changes from this PR?

yup, i'll work on the docs.

rebello95 · 2020-08-11T17:59:33Z

Thanks!

jingwei99 force-pushed the jh-ms/stats-poc branch from 7202a8d to 37370d5 Compare July 31, 2020 00:08

goaway reviewed Jul 31, 2020

View reviewed changes

library/common/engine.cc Outdated Show resolved Hide resolved

library/common/engine.cc Outdated Show resolved Hide resolved

rebello95 reviewed Jul 31, 2020

View reviewed changes

goaway changed the title ~~Jh ms/stats poc~~ stats: introduce stats client Aug 1, 2020

goaway marked this pull request as ready for review August 1, 2020 07:35

goaway and others added 11 commits August 4, 2020 18:30

wip

c7d99af

Co-authored-by: Jingwei <57155915+jingwei99@users.noreply.github.com> Signed-off-by: Mike Schore <mike.schore@gmail.com>

update

8c3c0cd

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

update interfaces and implementations

b804494

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

more files

fab1505

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

update interface

0213f89

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

comments

b54bd6a

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

wip updating Swift interfaces

09c6cc1

Signed-off-by: Mike Schore <mike.schore@gmail.com>

cleanup

4ac676d

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add strong type of element for android

4801a0b

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

doc for Element

7f71fe5

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

format

843175a

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway force-pushed the jh-ms/stats-poc branch from 6cc46a8 to 843175a Compare August 4, 2020 10:34

goaway and others added 2 commits August 4, 2020 13:23

add Swift Element

7d363fb

Signed-off-by: Mike Schore <mike.schore@gmail.com>

edit after rebase, forqak android

3ded59b

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

jingwei99 force-pushed the jh-ms/stats-poc branch from c0a17d6 to 3ded59b Compare August 5, 2020 16:13

Jingwei Hao and others added 2 commits August 5, 2020 18:25

fix ci (mostly) for android

2d8aa5a

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

fix up swift Element

356390c

Signed-off-by: Mike Schore <mike.schore@gmail.com>

buildbreaker reviewed Aug 6, 2020

View reviewed changes

examples/java/hello_world/MainActivity.java Outdated Show resolved Hide resolved

goaway added 3 commits August 7, 2020 01:48

formatting/lint

8ab5ad2

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix iOS example apps

de28910

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix test

352fd28

Signed-off-by: Mike Schore <mike.schore@gmail.com>

rebello95 reviewed Aug 6, 2020

View reviewed changes

junr03 reviewed Aug 6, 2020

View reviewed changes

comments

d977737

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

Jingwei Hao and others added 11 commits August 7, 2020 17:01

Merge branch 'main' into jh-ms/stats-poc

ec8f26a

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

examples

bad8422

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

lint

a8d0bbb

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

add Swift tests for StatsClientImpl

e75dbeb

Signed-off-by: Mike Schore <mike.schore@gmail.com>

swiftlint fixes

3962e96

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix swift example app

827d693

Signed-off-by: Mike Schore <mike.schore@gmail.com>

update and align docstrings and comments

0a216a5

Signed-off-by: Mike Schore <mike.schore@gmail.com>

rename stat directory to stats

e89dfa0

Signed-off-by: Mike Schore <mike.schore@gmail.com>

don't override CustomStringConvertible

9fdaaca

Signed-off-by: Mike Schore <mike.schore@gmail.com>

couple more updates for comments

d3b575b

Signed-off-by: Mike Schore <mike.schore@gmail.com>

make clients be properties on swift Engine

b623969

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested review from rebello95, junr03 and buildbreaker August 10, 2020 21:39

add experimental callout

6ae7be3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

junr03 approved these changes Aug 10, 2020

View reviewed changes

jmarantz reviewed Aug 10, 2020

View reviewed changes

rebello95 reviewed Aug 10, 2020

View reviewed changes

Jingwei Hao added 2 commits August 10, 2020 19:17

comments

7fc3ec0

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

andr

c089d69

Signed-off-by: Jingwei Hao <jingweih@lyft.com>

jingwei99 requested a review from rebello95 August 11, 2020 16:35

jingwei99 commented Aug 11, 2020

View reviewed changes

rebello95 approved these changes Aug 11, 2020

View reviewed changes

goaway merged commit b9a4334 into main Aug 11, 2020

goaway deleted the jh-ms/stats-poc branch August 11, 2020 17:50

goaway mentioned this pull request Aug 12, 2020

stats: add scope for external stats #1020

Merged

		/// - parameter elements: Elements to identify a counter
		/// - returns: A Counter based on the joined elements.

	func testCounterDelegatesToEngine() throws {
	func testCounterDelegatesToEngine() {

	func testCounterDelegatesToEngineWithCount() throws {
	func testCounterDelegatesToEngineWithCount() {

	func testCounterWeaklyHoldsEngine() throws {
	func testCounterWeaklyHoldsEngine() {

stats: introduce stats client #992

stats: introduce stats client #992

Conversation

jingwei99 commented Jul 30, 2020 • edited by goaway

rebello95 left a comment

Choose a reason for hiding this comment

goaway commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingwei99 commented Aug 11, 2020

Choose a reason for hiding this comment

rebello95 commented Aug 11, 2020

jingwei99 commented Aug 11, 2020

rebello95 commented Aug 11, 2020

jingwei99 commented Jul 30, 2020 •

edited by goaway