kennknowles commented on code in PR #36962:
URL: https://github.com/apache/beam/pull/36962#discussion_r2599223343
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -857,8 +857,13 @@ class BeamModulePlugin implements Plugin<Project> {
netty_tcnative_boringssl_static :
"io.netty:netty-tcnative-boringssl-static:2.0.52.Final",
netty_transport :
"io.netty:netty-transport:$netty_version",
netty_transport_native_epoll :
"io.netty:netty-transport-native-epoll:$netty_version",
- 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",
Review 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.
##########
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/BatchViewOverrides.java:
##########
@@ -1403,6 +1404,11 @@ public PaneInfo getPaneInfo() {
return null;
}
+ @Override
+ public @Nullable Context getContext() {
Review Comment:
`getContext` is pretty generic, even though it is accurate in this context
(pun intended). Perhaps `getOpenTelemetryContext` even though it is longer
it'll be more readable.
##########
sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java:
##########
@@ -58,6 +58,7 @@ public void testGcpCoreApiSurface() throws Exception {
classesInPackage("com.fasterxml.jackson.annotation"),
classesInPackage("com.google.cloud.hadoop.gcsio"),
classesInPackage("com.google.common.collect"), // Via
gcs-connector ReadOptions builder
+ classesInPackage("io.opentelemetry"), // open telemetry
Review Comment:
This is fine. I actually think this test code-rotted some time ago, and also
its purpose is somewhat obsolete: Before we released "vendor" versions of Guava
and Calcite and other things, we relocated and included them in the built jars
dynamically - so our codebase _looked_ like it linked to the normal version,
but during jar assembly it was relocated to the vendor namespace. So we
accidentally could ship something with an API that could never be used. This
test was to make sure we didn't do that. Now that we (mostly) vendor things, it
is less important. We probably still have some uses of dynamic relocation, but
it isn't "everything/everywhere" like it used to be.
(I had a philosophy, which turns out to be a PITA, that we should relocate
everything except for allowlisted API surface classes, to keep dependencies to
only actual public deps. This is slow, causes massive bloat, and IDEs do not
understand it. I still like the idea that "implementation detail" dependencies
do not impact users but it needs language and runtime support, not hacking in
the build system - the "right" way in Java is probably classloaders, ugh)
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java:
##########
@@ -996,6 +1089,51 @@ public List<? extends Coder<?>> getComponents() {
}
}
+ public static class OpenTelemetryContextSerializer {
+
+ private static final
TextMapSetter<BeamFnApi.Elements.ElementMetadata.Builder> SETTER =
+ (carrier, key, value) -> {
+ if (carrier == null) {
+ return;
+ }
+ if ("traceparent".equals(key)) {
+ carrier.setTraceparent(value);
+ } else if ("tracestate".equals(key)) {
+ carrier.setTracestate(value);
+ }
+ };
+
+ private static final TextMapGetter<BeamFnApi.Elements.ElementMetadata>
GETTER =
+ new TextMapGetter<BeamFnApi.Elements.ElementMetadata>() {
+ @Override
+ public Iterable<String> keys(BeamFnApi.Elements.ElementMetadata
carrier) {
+ return Lists.newArrayList("traceparent", "tracestate");
+ }
+
+ @Override
+ public @Nullable String get(
+ BeamFnApi.Elements.@Nullable ElementMetadata carrier, String
key) {
+ if (carrier == null) {
+ return null;
+ }
+ if ("traceparent".equals(key)) {
+ return carrier.getTraceparent();
+ } else if ("tracestate".equals(key)) {
+ return carrier.getTracestate();
+ }
+ return null;
+ }
+ };
+
+ public static void write(Context from,
BeamFnApi.Elements.ElementMetadata.Builder builder) {
+ W3CTraceContextPropagator.getInstance().inject(from, builder, SETTER);
+ }
+
+ public static Context read(BeamFnApi.Elements.ElementMetadata from) {
+ return W3CTraceContextPropagator.getInstance().extract(Context.root(),
from, GETTER);
Review 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
```java
W3TraceContextPropagator.getInstance().setTracestate(elementMetadata.getTracestate());
W3TraceContextPropagator.getInstance().setTraceparent(elementMetadata.getTraceparent());
```
and
```java
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.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java:
##########
@@ -77,7 +82,11 @@ public static <T> Builder<T> builder(WindowedValue<T>
template) {
.setValue(template.getValue())
.setTimestamp(template.getTimestamp())
.setWindows(template.getWindows())
- .setPaneInfo(template.getPaneInfo());
+ .setPaneInfo(template.getPaneInfo())
+ .setContext(template.getContext())
+ .setCausedByDrain(template.causedByDrain())
Review Comment:
All this propagation of other fields would be valuable to commit separately
for code history. Perhaps `jj split` these to other PRs, or make them tiny
commits in this PR.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java:
##########
@@ -996,6 +1089,51 @@ public List<? extends Coder<?>> getComponents() {
}
}
+ public static class OpenTelemetryContextSerializer {
Review Comment:
`private`? Possibly package level access (maybe it is just
`@VisibleForTesting`? I didn't audit the whole PR...
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/WindowedValues.java:
##########
@@ -933,16 +1023,18 @@ public void encode(WindowedValue<T> windowedElem,
OutputStream outStream, Contex
@Override
public WindowedValue<T> decode(InputStream inStream) throws
CoderException, IOException {
- return decode(inStream, Context.NESTED);
+ return decode(inStream, org.apache.beam.sdk.coders.Coder.Context.NESTED);
Review Comment:
Should be able to be `Coder.Context` if you want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
