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
stats: introduce stats client #992
Changes from all commits
c7d99af
8c3c0cd
b804494
fab1505
0213f89
b54bd6a
09c6cc1
4ac676d
4801a0b
7f71fe5
843175a
7d363fb
3ded59b
2d8aa5a
356390c
8ab5ad2
de28910
352fd28
d977737
ec8f26a
bad8422
a8d0bbb
e75dbeb
3962e96
827d693
0a216a5
e89dfa0
9fdaaca
d3b575b
b623969
6ae7be3
7fc3ec0
c089d69
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 |
---|---|---|
|
@@ -93,6 +93,19 @@ Engine::~Engine() { | |
main_thread_.join(); | ||
} | ||
|
||
void Engine::recordCounter(std::string elements, uint64_t count) { | ||
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. Can we cover this in tests? 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. Adding a ticket to cover testing generally at this layer, will link here. 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. @goaway don't forget to add this ticket 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. Created (but it's internal). |
||
if (server_) { | ||
server_->dispatcher().post([this, elements, count]() -> void { | ||
static const std::string client = "client"; | ||
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. 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. |
||
absl::string_view prefix{client}; | ||
goaway marked this conversation as resolved.
Show resolved
Hide resolved
|
||
absl::string_view dynamic_elements{elements}; | ||
Stats::Utility::counterFromElements(server_->serverFactoryContext().scope(), | ||
goaway marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{prefix, dynamic_elements}) | ||
.add(count); | ||
}); | ||
} | ||
} | ||
|
||
void Engine::flushStats() { | ||
// The server will be null if the post-init callback has not been completed within run(). | ||
// In this case, we can simply ignore the flush. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ android_artifacts( | |
kt_android_library( | ||
name = "envoy_lib", | ||
srcs = [ | ||
"AndroidStreamClientBuilder.kt", | ||
"AndroidEngineBuilder.kt", | ||
], | ||
custom_package = "io.envoyproxy.envoymobile", | ||
manifest = "EnvoyManifest.xml", | ||
|
@@ -36,7 +36,9 @@ kt_android_library( | |
envoy_mobile_kt_library( | ||
name = "envoy_interfaces_lib", | ||
srcs = glob([ | ||
"EnvoyClient.kt", | ||
"Engine.kt", | ||
"EngineBuilder.kt", | ||
"EngineImpl.kt", | ||
"EnvoyError.kt", | ||
"Headers.kt", | ||
"HeadersBuilder.kt", | ||
|
@@ -52,15 +54,18 @@ envoy_mobile_kt_library( | |
"ResponseTrailersBuilder.kt", | ||
"RetryPolicy.kt", | ||
"RetryPolicyMapper.kt", | ||
"StatsClient.kt", | ||
"StatsClientImpl.kt", | ||
Comment on lines
+57
to
+58
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. Shouldn't these be in 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. I think these ideally live here, basically in the same level as |
||
"Stream.kt", | ||
"StreamClient.kt", | ||
"StreamClientBuilder.kt", | ||
"StreamCallbacks.kt", | ||
"StreamClient.kt", | ||
"StreamClientImpl.kt", | ||
"StreamPrototype.kt", | ||
"UpstreamHttpProtocol.kt", | ||
"filters/*.kt", | ||
"grpc/*.kt", | ||
"mocks/*.kt", | ||
"stats/*.kt", | ||
]), | ||
visibility = ["//visibility:public"], | ||
deps = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package io.envoyproxy.envoymobile | ||
|
||
/** | ||
* Engine represents a running instance of Envoy Mobile, and provides client interfaces that run on | ||
* that instance. | ||
*/ | ||
interface Engine { | ||
|
||
/** | ||
* @return a {@link StreamClient} for opening and managing HTTP streams. | ||
*/ | ||
fun streamClient(): StreamClient | ||
|
||
/** | ||
* @return a {@link StatsClient} for recording time series metrics. | ||
*/ | ||
fun statsClient(): StatsClient | ||
} |
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'd prefer having a more restrictive rule so that people have to think before their application level stats get emitted.
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.
what rules do you have in mind?
right now all the elements used for the stats are checked against the regex:
^[A-Za-z_]+\$
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.
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.