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: Introduce OpenTelemetry Metrics Recording #2500

Merged
merged 28 commits into from
Mar 14, 2024

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Feb 23, 2024

Builds off #2433, based on the design in go/java-gapic-otel-metrics-design.

Discovered two issues via showcase tests:

  1. Otel's Method Name is different between gRPC and HttpJson #2502
  2. HttpJson Implementation is recording Operation(Success|Fail) metric before retries are attempted #2503

These issues are not blocking for this PR.

@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Feb 23, 2024
@lqiu96 lqiu96 changed the title feat: Introduce Metrics Recording via OpenTelemetry feat: Introduce OpenTelemetry Metrics Recording Feb 26, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Feb 26, 2024
@lqiu96 lqiu96 force-pushed the implement-OpentelemetryMetricsRecorder branch from 50fa91a to 21407aa Compare February 26, 2024 17:04
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Feb 26, 2024
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Feb 27, 2024
Comment on lines 120 to 136
@Override
public void recordOperationLatency(double operationLatency, Map<String, String> attributes) {
operationLatencyRecorder.record(operationLatency, toOtelAttributes(attributes));
}

/**
* Record an operation made. The operation count number is stored in a LongCounter.
*
* <p>The operation count should always be 1 and this should be invoked once.
*
* @param count The number of operations made
* @param attributes Map of the attributes to store
*/
@Override
public void recordOperationCount(long count, Map<String, String> attributes) {
operationCountRecorder.add(count, toOtelAttributes(attributes));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both Operation record calls should only occur once. Do we want to validate this/ throw an exception if recordOperation(...) is invoked more than once?

Copy link
Collaborator

@blakeli0 blakeli0 Mar 5, 2024

Choose a reason for hiding this comment

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

I think we should rely on the existing framework. If there is a existing bug, it should not be the responsibility of the recorder to validate it as well, we should probably validate it in the MetricsTracer.

Comment on lines 120 to 136
@Override
public void recordOperationLatency(double operationLatency, Map<String, String> attributes) {
operationLatencyRecorder.record(operationLatency, toOtelAttributes(attributes));
}

/**
* Record an operation made. The operation count number is stored in a LongCounter.
*
* <p>The operation count should always be 1 and this should be invoked once.
*
* @param count The number of operations made
* @param attributes Map of the attributes to store
*/
@Override
public void recordOperationCount(long count, Map<String, String> attributes) {
operationCountRecorder.add(count, toOtelAttributes(attributes));
}
Copy link
Collaborator

@blakeli0 blakeli0 Mar 5, 2024

Choose a reason for hiding this comment

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

I think we should rely on the existing framework. If there is a existing bug, it should not be the responsibility of the recorder to validate it as well, we should probably validate it in the MetricsTracer.

gax-java/pom.xml Outdated Show resolved Hide resolved
showcase/pom.xml Outdated Show resolved Hide resolved
@lqiu96 lqiu96 requested a review from blakeli0 March 5, 2024 23:08
public OpenTelemetryMetricsRecorder(OpenTelemetry openTelemetry, String serviceName) {
Meter meter =
openTelemetry
.meterBuilder("Gax-OtelMetrics")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use gax-java. Based on the Javadoc, the value here should be the library name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated.

@blakeli0 blakeli0 added the automerge Merge the pull request once unit tests and other checks pass. label Mar 14, 2024
Copy link

Quality Gate Failed Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@blakeli0 blakeli0 merged commit b936580 into main Mar 14, 2024
43 of 44 checks passed
@blakeli0 blakeli0 deleted the implement-OpentelemetryMetricsRecorder branch March 14, 2024 20:17
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 14, 2024
diegomarquezp pushed a commit that referenced this pull request Mar 15, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.38.0</summary>

##
[2.38.0](v2.37.0...v2.38.0)
(2024-03-15)


### Features

* [common-protos] add `api_version` extension to `ServiceOptions`, for
collaborative versioning
([d343be9](d343be9))
* [common-protos] add `api_version` extension to `ServiceOptions`, for
collaborative versioning
([#2551](#2551))
([d343be9](d343be9))
* add `ErrorReason.LOCATION_POLICY_VIOLATED` enum value
([d343be9](d343be9))
* add `ErrorReason.LOCATION_POLICY_VIOLATED` enum value
([d343be9](d343be9))
* add `Publishing.rest_reference_documentation_uri` to aid client
library publication
([d343be9](d343be9))
* add `Publishing.rest_reference_documentation_uri` to aid client
library publication
([d343be9](d343be9))
* Add shopping and chat common protos.
([#2553](#2553))
([5f2d4e7](5f2d4e7)),
closes
[#2018](#2018)
* get PR description from googleapis commits
([#2531](#2531))
([c2ea697](c2ea697))
* Introduce OpenTelemetry Metrics Recording
([#2500](#2500))
([b936580](b936580))
* skip build only commit
([#2555](#2555))
([180c8a9](180c8a9))
* Universe Domain Environment Variable Support
([#2485](#2485))
([1463d64](1463d64))


### Dependencies

* normalize dependencies
([#2574](#2574))
([6622238](6622238))
* update arrow.version to v15.0.1
([#2565](#2565))
([b2c3f6a](b2c3f6a))
* update dependency com.fasterxml.jackson:jackson-bom to v2.17.0
([#2564](#2564))
([40ae7f9](40ae7f9))
* update dependency com.google.api-client:google-api-client-bom to
v2.4.0
([#2570](#2570))
([f60441f](f60441f))
* update dependency com.google.errorprone:error_prone_annotations to
v2.26.1
([#2530](#2530))
([7c1aaab](7c1aaab))
* update dependency com.google.errorprone:error_prone_annotations to
v2.26.1
([#2532](#2532))
([447b4e1](447b4e1))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.65.final
([#2547](#2547))
([46e0e0f](46e0e0f))
* update dependency net.bytebuddy:byte-buddy to v1.14.12
([#2522](#2522))
([edfec32](edfec32))
* update google api dependencies
([#2484](#2484))
([92e91bc](92e91bc))
* update google api dependencies
([#2538](#2538))
([d9355cf](d9355cf))
* update googleapis/java-cloud-bom digest to 3f93d58
([#2499](#2499))
([5fd4d5e](5fd4d5e))
* update googleapis/java-cloud-bom digest to 659764f
([#2545](#2545))
([d6c8be6](d6c8be6))
* update netty dependencies
([#2480](#2480))
([40753c3](40753c3))
* update opentelemetry-java monorepo to v1.35.0
([#2477](#2477))
([3ecefff](3ecefff))
* update opentelemetry-java monorepo to v1.36.0
([#2550](#2550))
([9669c21](9669c21))
* update opentelemetry-java monorepo to v1.36.0
([#2573](#2573))
([f5f201e](f5f201e))
* update slf4j monorepo to v2.0.12
([#2481](#2481))
([363a354](363a354))


### Documentation

* minor tweaks to various comments
([d343be9](d343be9))
* minor tweaks to various comments
([d343be9](d343be9))
</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
Labels
size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants