[OpenTelemetry] Model changes to allow OpenTelemetry context propagation#36962
[OpenTelemetry] Model changes to allow OpenTelemetry context propagation#36962stankiewicz wants to merge 3 commits intoapache:masterfrom
Conversation
|
R: @sjvanrossum |
Summary of ChangesHello @stankiewicz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational groundwork for integrating OpenTelemetry context propagation within Apache Beam. It involves updating OpenTelemetry dependencies, extending the internal data model ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request effectively integrates OpenTelemetry context propagation into Apache Beam's core data structures and serialization mechanisms. This foundational change is well-implemented across various files, ensuring the Context is carried alongside WindowedValue elements. Key aspects include updating dependencies, extending the protocol buffer schema for traceparent and tracestate, modifying core WindowedValue interfaces and implementations to include the Context, and implementing robust serialization/deserialization logic via OpenTelemetryContextSerializer. The addition of a test case in WindowedValueTest.java validates the new functionality. The use of null for getContext() in specific overrides is consistent with the stated scope that the context is not yet consumed by users. Overall, this is a solid and necessary step towards comprehensive OpenTelemetry integration.
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
7be5980 to
69f9591
Compare
9c7ed81 to
43f92ec
Compare
...ud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
Outdated
Show resolved
Hide resolved
...ud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
Outdated
Show resolved
Hide resolved
| opentelemetry_api : "io.opentelemetry:opentelemetry-api", // google_cloud_platform_libraries_bom sets version | ||
| opentelemetry_api : "io.opentelemetry:opentelemetry-api:$opentelemetry_version", | ||
| opentelemetry_bom : "io.opentelemetry:opentelemetry-bom-alpha:$opentelemetry_version-alpha", // alpha required by extensions | ||
| opentelemetry_context : "io.opentelemetry:opentelemetry-context:$opentelemetry_version", |
There was a problem hiding this comment.
Will this change conflict with the GCP BOM? @suztomo do you think this is fine? We might just need to check them together manually.
...e-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/BatchViewOverrides.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static Context read(BeamFnApi.Elements.ElementMetadata from) { | ||
| return W3CTraceContextPropagator.getInstance().extract(Context.root(), from, GETTER); |
There was a problem hiding this comment.
OK... this way of doing things matches https://opentelemetry.io/docs/specs/otel/context/api-propagators/
But it does seem like a complicated way of doing the equivalent of
W3TraceContextPropagator.getInstance().setTracestate(elementMetadata.getTracestate());
W3TraceContextPropagator.getInstance().setTraceparent(elementMetadata.getTraceparent());and
elementMetadataBuilder.setTracestate(W3TraceContextPropagator.getInstance().getTracestate());
elementMetadataBuilder.setTraceparent(W3TraceContextPropagator.getInstance().getTraceparent());Other nit: did we not agree that the ElementMetadata object should be only implementation detail? This class and everything should be private since it is just for encoding/decoding utility. But potentially also you want to do the SETTER on an Builder and the GETTER on a WindowedValue. If it is implementation detail, hidden, then I don't care that much, but if it is public then we have to be careful about what classes we use.
There was a problem hiding this comment.
Yes, but I don't want to reimplement W3CTraceContextPropagator to have getTraceState /getTraceParent methods.
On nits - ValueInSingleWindow is not implementing WindowedValue interface now. Once it will, I can improve that.
Setter has builder already.
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/ValueInSingleWindow.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValue.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
a0171cc to
3afc30b
Compare
4de3025 to
329eef6
Compare
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
need to resolve conflicts and it will be ready soon. |
|
Run Java_IOs_Direct PreCommit |
|
Run Java PreCommit |
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValue.java
Show resolved
Hide resolved
33b239d to
6ab05e2
Compare
|
Run Java PreCommit |
sjvanrossum
left a comment
There was a problem hiding this comment.
Minor feedback, LGTM overall.
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
Outdated
Show resolved
Hide resolved
| opentelemetry_api : "io.opentelemetry:opentelemetry-api:$opentelemetry_sdk_version", | ||
| opentelemetry_bom : "io.opentelemetry:opentelemetry-bom-alpha:$opentelemetry_sdk_version-alpha", // alpha required by extensions | ||
| opentelemetry_context : "io.opentelemetry:opentelemetry-context:$opentelemetry_sdk_version", | ||
| opentelemetry_gcp_auth : "io.opentelemetry.contrib:opentelemetry-gcp-auth-extension:$opentelemetry_contrib_version-alpha", | ||
| opentelemetry_sdk : "io.opentelemetry:opentelemetry-sdk:$opentelemetry_sdk_version", | ||
| opentelemetry_exporter_otlp : "io.opentelemetry:opentelemetry-exporter-otlp:$opentelemetry_sdk_version", | ||
| opentelemetry_extension_autoconfigure : "io.opentelemetry:opentelemetry-sdk-extension-autoconfigure:$opentelemetry_sdk_version", |
There was a problem hiding this comment.
GCP BOM sets OpenTelemetry SDK version iirc:
| opentelemetry_api : "io.opentelemetry:opentelemetry-api:$opentelemetry_sdk_version", | |
| opentelemetry_bom : "io.opentelemetry:opentelemetry-bom-alpha:$opentelemetry_sdk_version-alpha", // alpha required by extensions | |
| opentelemetry_context : "io.opentelemetry:opentelemetry-context:$opentelemetry_sdk_version", | |
| opentelemetry_gcp_auth : "io.opentelemetry.contrib:opentelemetry-gcp-auth-extension:$opentelemetry_contrib_version-alpha", | |
| opentelemetry_sdk : "io.opentelemetry:opentelemetry-sdk:$opentelemetry_sdk_version", | |
| opentelemetry_exporter_otlp : "io.opentelemetry:opentelemetry-exporter-otlp:$opentelemetry_sdk_version", | |
| opentelemetry_extension_autoconfigure : "io.opentelemetry:opentelemetry-sdk-extension-autoconfigure:$opentelemetry_sdk_version", | |
| opentelemetry_api : "io.opentelemetry:opentelemetry-api", // google_cloud_platform_libraries_bom sets version | |
| opentelemetry_bom : "io.opentelemetry:opentelemetry-bom-alpha:$opentelemetry_sdk_version-alpha", // alpha required by extensions | |
| opentelemetry_context : "io.opentelemetry:opentelemetry-context", // google_cloud_platform_libraries_bom sets version | |
| opentelemetry_gcp_auth : "io.opentelemetry.contrib:opentelemetry-gcp-auth-extension:$opentelemetry_contrib_version-alpha", | |
| opentelemetry_sdk : "io.opentelemetry:opentelemetry-sdk", // google_cloud_platform_libraries_bom sets version | |
| opentelemetry_exporter_otlp : "io.opentelemetry:opentelemetry-exporter-otlp", // google_cloud_platform_libraries_bom sets version | |
| opentelemetry_extension_autoconfigure : "io.opentelemetry:opentelemetry-sdk-extension-autoconfigure", // google_cloud_platform_libraries_bom sets version |
| BoundedWindow window = windowCoder.decode(inStream); | ||
| PaneInfo paneInfo = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream); | ||
| CausedByDrain causedByDrain = CausedByDrain.NORMAL; | ||
| io.opentelemetry.context.@Nullable Context openTelemetryContext = null; |
There was a problem hiding this comment.
| io.opentelemetry.context.@Nullable Context openTelemetryContext = null; | |
| @Nullable io.opentelemetry.context.Context openTelemetryContext = null; |
There was a problem hiding this comment.
it's valid place for this annotation. It looks strange.
There was a problem hiding this comment.
As long as Spotless approves it's fine with me, but it seemed unconventional.
Fun party trick though, I didn't know this was syntactically allowed until I saw it pop up in this PR. 😄
There was a problem hiding this comment.
It looks even weirder when it is an inner class (OuterClass.@Nullable InnerClass). Not only is it conventional, it is required. (there is no other valid syntax for this)
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java
Outdated
Show resolved
Hide resolved
|
|
||
| class OpenTelemetryContextPropagator { | ||
|
|
||
| private static final TextMapSetter<BeamFnApi.Elements.ElementMetadata.Builder> SETTER = |
There was a problem hiding this comment.
I feel like errorprone is going to tell you to make this a static method and use a method reference.
There was a problem hiding this comment.
it doesn't because TextMapSetter is custom interface. I will keep it as is, as Getter is not a lambda.
| } | ||
| }; | ||
|
|
||
| private static final TextMapGetter<BeamFnApi.Elements.ElementMetadata> GETTER = |
sdks/java/core/src/main/java/org/apache/beam/sdk/values/ValueInSingleWindow.java
Outdated
Show resolved
Hide resolved
| BoundedWindow window = windowCoder.decode(inStream); | ||
| PaneInfo paneInfo = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream); | ||
| CausedByDrain causedByDrain = CausedByDrain.NORMAL; | ||
| io.opentelemetry.context.@Nullable Context openTelemetryContext = null; |
There was a problem hiding this comment.
It looks even weirder when it is an inner class (OuterClass.@Nullable InnerClass). Not only is it conventional, it is required. (there is no other valid syntax for this)
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValue.java
Show resolved
Hide resolved
spotless. rename fields, cleanup groovy spotless
f2dd0bc to
ee77bb1
Compare
|
Run Java PreCommit |
|
Run Java_GCP_IO_Direct PreCommit |
Changes:
Next PRs:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.