Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
stats: introduce stats client #992
Changes from 15 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
There are no files selected for viewing
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.
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.
Can we cover this in tests?
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Created (but it's internal).
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.
Shouldn't these be in
stats/
which is captured below with globbing?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 think these ideally live here, basically in the same level as
StreamClient
, as the interfaceEngine.kt
exposes aStatsClient
and aStreamClient
.