-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
50fa91a
to
21407aa
Compare
gax-java/gax/src/test/java/com/google/api/gax/tracing/OpentelemetryMetricsRecorderTest.java
Outdated
Show resolved
Hide resolved
@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)); | ||
} |
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.
Both Operation record calls should only occur once. Do we want to validate this/ throw an exception if recordOperation(...) is invoked more than once?
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 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
.
showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpentelemetryMetricsRecorder.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpentelemetryMetricsRecorder.java
Outdated
Show resolved
Hide resolved
@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)); | ||
} |
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 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/gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java
Outdated
Show resolved
Hide resolved
public OpenTelemetryMetricsRecorder(OpenTelemetry openTelemetry, String serviceName) { | ||
Meter meter = | ||
openTelemetry | ||
.meterBuilder("Gax-OtelMetrics") |
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.
Let's use gax-java
. Based on the Javadoc, the value here should be the library name.
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.
Sure, updated.
Co-authored-by: Blake Li <blakeli@google.com>
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryMetricsRecorder.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryMetricsRecorder.java
Show resolved
Hide resolved
…metryMetricsRecorder.java
Quality Gate failed for 'gapic-generator-java-root'Failed conditions |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
🤖 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>
Builds off #2433, based on the design in go/java-gapic-otel-metrics-design.
Discovered two issues via showcase tests:
These issues are not blocking for this PR.