feat(bigtable): add support for request stats #6991
Conversation
humbertowastaken
requested review from
enocom,
telpirion and
a team
as code owners
product-auto-label
bot
added
size: s
|
This PR also requires a unit test and an integration test. |
test(bigtable): add integration tests for request stats
product-auto-label
bot
added
size: m
|
Added integration tests. |
There was a problem hiding this comment.
I think this still needs a unit test in bigtable_test.go. The Go monorepo doesn't run integration tests for each PR until it has been merged. As a result, we need to ensure that unit tests exist to help us see breakages before a PR is merged.
In sum, bare minimum required changes:
- unit test(s) for this callback feature in
bigtable_test.go - comments for all publicly exposed fields to populate the reference docs
| } | ||
|
|
||
| func makeFullReadStats(reqStats *btpb.RequestStats) FullReadStats { | ||
| statsView := reqStats.GetFullReadStatsView() |
There was a problem hiding this comment.
Although this is guarded by an if statement above, I recommend a nil check here.
There was a problem hiding this comment.
What would you recommend to do in the nil case? Since this is an internal impl detail, we can update this in a follow up PR
telpirion
added
the
do not merge
product-auto-label
bot
added
size: l
igorbernstein2
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
|
At this point the only thing that missing is a unit test. This will require some refactoring in the emulator code, which we can do in a follow up PR. The functionality is covered by the integration test. Given the timing, I'm going to go ahead and merge this. |
igorbernstein2
dismissed
telpirion’s stale review
The only outstanding action item is a unit test which will come in a follow PR
igorbernstein2
removed
the
do not merge
feat(bigtable): add support for request stats