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

feat: Update opencensus metrics to include bigtable resource ids and rpc level metrics #214

Merged
merged 9 commits into from May 18, 2020

Conversation

igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Mar 5, 2020

This PR refactors opencensus metrics integration to use gax's ApiTracers.
Which allows this client to instrument individual attempts and tag everything
with bigtable resource ids

Note: clirr is complaining about removed classes, but they are all marked as InternalApi

@googlebot googlebot added the cla: yes label Mar 5, 2020
@kolea2 kolea2 added the release-please:force-run label Mar 16, 2020
@release-please release-please bot removed the release-please:force-run label Mar 16, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 18, 2020

Codecov Report

Merging #214 into master will increase coverage by 0.79%.
The diff coverage is 92.27%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #214      +/-   ##
============================================
+ Coverage     79.89%   80.69%   +0.79%     
- Complexity      992     1049      +57     
============================================
  Files            99       99              
  Lines          6447     6541      +94     
  Branches        340      344       +4     
============================================
+ Hits           5151     5278     +127     
+ Misses         1118     1082      -36     
- Partials        178      181       +3     
Impacted Files Coverage Δ Complexity Δ
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 95.04% <ø> (-0.11%) 20.00 <0.00> (ø)
...d/bigtable/data/v2/stub/metrics/MetricsTracer.java 78.48% <78.48%> (ø) 14.00 <14.00> (?)
...able/data/v2/stub/metrics/RpcMeasureConstants.java 92.85% <92.85%> (+1.94%) 1.00 <1.00> (ø)
...ud/bigtable/data/v2/stub/EnhancedBigtableStub.java 95.74% <100.00%> (+0.47%) 21.00 <0.00> (ø)
...bigtable/data/v2/stub/metrics/CompositeTracer.java 100.00% <100.00%> (ø) 33.00 <33.00> (?)
...e/data/v2/stub/metrics/CompositeTracerFactory.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...ble/data/v2/stub/metrics/MetricsTracerFactory.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...igtable/data/v2/stub/metrics/RpcViewConstants.java 97.67% <100.00%> (+97.67%) 1.00 <0.00> (+1.00)
.../cloud/bigtable/data/v2/stub/metrics/RpcViews.java 66.66% <100.00%> (+66.66%) 3.00 <1.00> (+3.00)
...le/cloud/bigtable/data/v2/models/BulkMutation.java 97.56% <0.00%> (-2.44%) 10.00% <0.00%> (ø%)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc215c5...787bc59. Read the comment docs.

@igorbernstein2 igorbernstein2 force-pushed the metrics-rewrite branch 2 times, most recently from cd9d8a5 to c383131 Compare Apr 1, 2020
@igorbernstein2 igorbernstein2 changed the title Metrics rewrite feat: Update opencensus metrics to include bigtable resource ids and rpc level metrics Apr 1, 2020
@igorbernstein2 igorbernstein2 changed the title feat: Update opencensus metrics to include bigtable resource ids and rpc level metrics feat: Update opencensus metrics to include bigtable resource ids and rpc level metrics Apr 1, 2020
@igorbernstein2 igorbernstein2 marked this pull request as ready for review Apr 1, 2020
@igorbernstein2 igorbernstein2 requested a review from kolea2 Apr 1, 2020
…rpc level metrics

This PR refactors opencensus metrics integration to use gax's ApiTracers.
Which allows this client to instrument individual attempts and tag everything
with bigtable resource ids
kolea2
kolea2 approved these changes Apr 9, 2020
Copy link
Collaborator

@kolea2 kolea2 left a comment

LGTM, though I think this needs a rebase and code format fixes

kolea2
kolea2 previously requested changes Apr 9, 2020
Copy link
Collaborator

@kolea2 kolea2 left a comment

To get clirr to pass, please add a clirr-ignored-differences.xml file. Example: https://github.com/googleapis/java-trace/blob/master/grpc-google-cloud-trace-v1/clirr-ignored-differences.xml

@kolea2
Copy link
Collaborator

@kolea2 kolea2 commented May 13, 2020

Adding do not merge - want to go through the merge conflicts that were resolved to ensure I didn't miss anything in the process.

@kolea2 kolea2 added the do not merge label May 13, 2020
@@ -124,6 +124,10 @@
<groupId>io.opencensus</groupId>
<artifactId>opencensus-api</artifactId>
</dependency>
<dependency>
<groupId>io.opencensus</groupId>
<artifactId>opencensus-impl</artifactId>
Copy link
Contributor Author

@igorbernstein2 igorbernstein2 May 14, 2020

Choose a reason for hiding this comment

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

Sorry for being confusing before, but opencensus-impl should stay as a test dep. I just meant that impl-core should be transitive. As it stands endusers should add -impl to activate opencensus (I just wanted to avoid users having to specify both

@igorbernstein2
Copy link
Contributor Author

@igorbernstein2 igorbernstein2 commented May 14, 2020

I think this looks good, wanna take it for spin and merge it?

@kolea2
Copy link
Collaborator

@kolea2 kolea2 commented May 14, 2020

asking @chingor13 for a extra look

@kolea2 kolea2 dismissed their stale review May 14, 2020

stale

Copy link
Collaborator

@chingor13 chingor13 left a comment

LGTM

Is the CompositeTracer something we should look to upstream into gax that would be useful to others?

@kolea2 kolea2 removed the do not merge label May 18, 2020
@kolea2 kolea2 merged commit 7214ef6 into googleapis:master May 18, 2020
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants