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.

Sign up for GitHub

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

Jump to bottom

fix: BaseApiTracer to noop on attemptFailed via overloaded method call #3016

Merged
merged 2 commits into from
Jul 8, 2024
Merged

fix: BaseApiTracer to noop on attemptFailed via overloaded method call #3016

merged 2 commits into from
Jul 8, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Jul 5, 2024 •
edited
Loading

Fixes #3015
Fixes #3014

Gax tracing internally works with attemptFailedDuration, which defaults to a no-op. Downstream libraries use attemptFailed, which has a custom behavior. What happens when an attempt-failed event occurs is that attemptFailedDuration is called instead (in favor of using java.time methods internally). This fix makes attemptFailedDuration's behavior to delegate the logic to attemptFailed.

The downstreams will keep failing because the repos haven't got adapted to the new change in gax. See the follow ups below.

Fixes in java-spanner

image

Fixes in java-bigtable

image

Follow ups in java-bigtable

More failures in java-bigtable to be addressed in that repo:

Error:    BigtableTableAdminSettingsTest.testToString:175 expected to contain: totalTimeout=PT13H32M
but was            : BigtableTableAdminSettings{projectId=our-project-85, instanceId=our-instance-06, 
...

Fixed in googleapis/java-bigtable#2274

Follow ups in java-spanner

Error:  Failures: 
Error:    CompositeTracerTest.testMethodsOverrideMetricsTracer:238 Method not found in compositeTracerMethods: public void com.google.api.gax.tracing.MetricsTracer.attemptFailedDuration(java.lang.Throwable,java.time.Duration)

Fixed in googleapis/java-spanner#3200

@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Jul 5, 2024
@diegomarquezp diegomarquezp changed the title fix: BaseApiTracer and derived classes to noop on attemptFailed via overloaded method call fix: BaseApiTracer to noop on attemptFailed via overloaded method call Jul 5, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 5, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 5, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@diegomarquezp diegomarquezp marked this pull request as ready for review July 5, 2024 00:36
@diegomarquezp diegomarquezp requested a review from a team as a code owner July 5, 2024 00:36
@diegomarquezp diegomarquezp mentioned this pull request Jul 5, 2024
Merged
@diegomarquezp diegomarquezp merged commit 2fc938a into main Jul 8, 2024
45 of 47 checks passed
@diegomarquezp diegomarquezp deleted the fix-downstreams-after-javatime branch July 8, 2024 13:46
@diegomarquezp
Copy link
Contributor Author

cc: @blakeli0 @lqiu96

Moving logic to ApiTracer (BaseApiTracer is left the same as in main)

java-spanner

image
image

java-bigtable

image
image
image

@diegomarquezp
Copy link
Contributor Author

Thanks @blakeli0. The suggestion to remove the overload in BaseApiTracer worked.
image
image

@diegomarquezp diegomarquezp mentioned this pull request Jul 9, 2024
Merged
diegomarquezp added a commit that referenced this pull request Jul 9, 2024
@diegomarquezp
chore: move the attemptFailed logic from BaseApiTracer to ApiTracer (…
b738de5 
…#3037)

Follow up of #3016

On `bigtable` and `spanner`:

![image](https://github.com/googleapis/sdk-platform-java/assets/22083784/696c0cec-e83b-4563-a9fe-7f6a249f7bc6)

![image](https://github.com/googleapis/sdk-platform-java/assets/22083784/6170cb3b-2e18-478c-b1ff-9cef5f869dd0)


Thanks @blakeli0 and @lqiu96 for the directions.
lqiu96 pushed a commit that referenced this pull request Jul 25, 2024
@release-please
chore(main): release 2.43.0 (#2953)
5e6da42 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.43.0</summary>

##
[2.43.0](v2.42.0...v2.43.0)
(2024-07-25)


### Features

* add `transport` option to `generation_config.yaml`
([#3052](#3052))
([3b1a915](3b1a915))
* get released version from versions.txt to render `README.md`
([#3007](#3007))
([99bb2b3](99bb2b3))
* Introduce java.time to Gax-Java
([#1872](#1872))
([308aeaf](308aeaf))
* Mark `getDefaultEndpoint()` with @ObsoleteApi
([#2347](#2347))
([e46648f](e46648f))
* parse `BUILD.bzel` to determine whether a commit that only changed
`BUILD.bazel` is a qualified commit
([#2937](#2937))
([502f801](502f801))


### Bug Fixes

* Fix:
([d996c2d](d996c2d))
* `BaseApiTracer` to noop on attemptFailed via overloaded method call
([#3016](#3016))
([2fc938a](2fc938a))
* Generator to skip generation for empty services.
([#3051](#3051))
([ff2c485](ff2c485))
* restore hermetic build image publication
([#2952](#2952))
([97a6d67](97a6d67))


### Dependencies

* update dependency com.fasterxml.jackson:jackson-bom to v2.17.2
([#3028](#3028))
([d16f9d1](d16f9d1))
* update dependency
com.google.cloud.opentelemetry:detector-resources-support to v0.30.0
([#2975](#2975))
([b3ec93f](b3ec93f))
* update dependency
com.google.cloud.opentelemetry:detector-resources-support to v0.31.0
([#3044](#3044))
([6bd07dc](6bd07dc))
* update dependency com.google.errorprone:error_prone_annotations to
v2.29.2
([#3058](#3058))
([8ea0868](8ea0868))
* update dependency com.google.errorprone:error_prone_annotations to
v2.29.2
([#3059](#3059))
([81b23dc](81b23dc))
* update dependency com.google.guava:guava to v33.2.1-jre
([#3027](#3027))
([12ee456](12ee456))
* update dependency commons-codec:commons-codec to v1.17.1
([#3049](#3049))
([58d94b7](58d94b7))
* update dependency dev.cel:cel to v0.6.0
([#3050](#3050))
([bc332d9](bc332d9))
* update dependency net.bytebuddy:byte-buddy to v1.14.18
([#3029](#3029))
([8799cf6](8799cf6))
* update dependency org.apache.commons:commons-lang3 to v3.15.0
([#3060](#3060))
([2538334](2538334))
* update dependency org.checkerframework:checker-qual to v3.45.0
([#2988](#2988))
([4edd216](4edd216))
* update google api dependencies
([#2951](#2951))
([c16f6c9](c16f6c9))
* update google auth library dependencies to v1.24.0
([#3039](#3039))
([98b5bd7](98b5bd7))
* update googleapis/java-cloud-bom digest to 47c5dbc
([#2974](#2974))
([57623f0](57623f0))
* update grpc dependencies to v1.65.1
([#3061](#3061))
([27497e2](27497e2))
* update junit5 monorepo to v5.10.3
([#2963](#2963))
([bc55fe1](bc55fe1))
* update netty dependencies to v4.1.112.final
([#3057](#3057))
([5af127b](5af127b))
* update opentelemetry-java monorepo to v1.40.0
([#3035](#3035))
([5c31c42](5c31c42))
* Use Gapic-Showcase v0.35.1
([#3018](#3018))
([43773f0](43773f0))


### Documentation

* add support option to 'new issue' choices
([#3055](#3055))
([6a2a17d](6a2a17d))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqiu96 lqiu96 lqiu96 approved these changes

@JoeWang1127 JoeWang1127 JoeWang1127 approved these changes

Assignees
No one assigned
Labels
size: xs Pull request size is extra small.
Projects
None yet
Milestone
No milestone
3 participants
@diegomarquezp @lqiu96 @JoeWang1127