Re-enable Agent Detection on Zos #13730

benjamin-confino · 2025-04-17T16:39:41Z

This resolves #13729

I have tested this locally and can confirm it works in our Z/OS test environment.

linux-foundation-easycla · 2025-04-17T16:39:47Z

The committers listed above are authorized under a signed CLA.

✅ login: benjamin-confino / name: Benjamin Confino (f9672ef, 2fec2e6, e71b5a4, 1435f3e)
✅ login: breedx-splk / name: jason plumb (a85fab6)
✅ login: laurit / name: Lauri Tulmin (ee6f9ff)

benjamin-confino · 2025-04-17T17:10:12Z

I'm reviewing the process to sign the CLA, I don't forsee any problems.

laurit · 2025-04-18T06:40:19Z

...rces/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java

-      return Files.lines(path);
+
+      String osName = getOsName();
+      if (!(osName.equalsIgnoreCase("z/OS") || osName.equalsIgnoreCase("OS/390"))) {


could switch the if and else parts to get rid of the negation

I put it this way around because making the default case the base one feels right, but I sense you have different tastes and its your codebase so I'll change it. (I'll test the updated version in our ZOS environment before pushing of course, which takes a bit of time)

...rces/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java

laurit · 2025-04-18T06:43:48Z

...rces/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java

+
+      // The odds of /proc being UTF-8 but the native encoding saying otherwise is extremely low
+      // but since UTF-8 is the standard it should be checked.
+      charsetsToTest.add(StandardCharsets.UTF_8);


does z/os ever use utf-8 for this file? assuming it does, when you'd attempt to parse utf-8 encoded file with Cp1047 would that ever fail? is it possible for the code flow to reach a point where it attempts utf-8 at all?

It cannot, and I did ask if there was any chance of it happening in the future and that's unlikely too. But I put this line in there anyway because I felt that the industry standard charset should always be on the list. If you disagree say the word and I'll remove it.

I think you should remove it.

laurit · 2025-04-18T06:45:12Z

...rces/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java

+      charsetsToTest.add(StandardCharsets.UTF_8);
+
+      IOException exception = null;
+      for (Charset charset : charsetsToTest) {


could you elaborate under what circumstances you'd expect parsing with one charset to fail and proceed to the next one?

Happy to. I'd expect this to fail on Z/OS if the JVM is configured with a non-default encoding on Z/OS. (I've had to do exactly that in our test environment)

In that case Charset.defaultCharset() will use whatever the user configured, if that is the wrong charset for reading /proc it will fall back to the next item which should pass since its hardcoded to Cp1047.

I did some tests with native.encoding on java17+ and couldn't change it from the default, but I'm not confident that I haven't missed something so its possible a similar scenario exists on java17+.

Lets assume that the application runs on jdk11 or 8 that relies on Charset.defaultCharset() and uses -Dfile.encoding=ISO-8859-1 (I think I saw somewhere a mention that websphere uses this). Now my guess would be that reading the file will succeed and the fallback to Cp1047 won't happen. The content of the read file will of course be messed up. If that is so then I think we need to come up with a different solution. Do you know is com.ibm.jzos.ZUtil.getDefaultPlatformEncoding() part of the java sdk on z/os? If it is we could add a compile dependency to https://central.sonatype.com/artifact/com.ibm.jzos/ibmjzos And use that to get the platform encoding.

I tested ISO-8859-1 and you're right, that is a fine catch that made it through all our automated tests. I also asked about getDefaultPlatformEncoding() but it turns out that also cannot guarantee correctness (especially if someone uses setDefaultPlatformEncoding() which is discouraged but has no enforcement against it).

The advice I was given was to keep it simple and just use Cp1047, as that would always be the case for /proc on ZOS. The other option was to read the file tag as /proc is all tagged with Cp1047.

Which of these would you prefer?

The advice I was given was to keep it simple and just use Cp1047, as that would always be the case for /proc on ZOS.

That reads to me as you should always use Cp1047 as this is known to be the encoding of that file and wouldn't need to look at native.encoding at all even when it is available. I'm not able to asses whether this is true or not. I'm willing to accept any solution that isn't obviously wrong. That includes unilaterally using Cp1047, falling back to Cp1047 when native.encoding is not available or using getDefaultPlatformEncoding().

Ok, I'll unilaterally use Cp1047. I've heard from two different Z/OS developers that /proc is always encoded that way so it should be safe. And it will remove the need for a whole method of fallbacks.

I ran the reworked version through our test system over the weekend it was fine so I've pushed it here, I've kept it in a separate commit for now for ease of reviewing.

laurit · 2025-04-18T06:47:52Z

.../library/src/test/java/io/opentelemetry/instrumentation/resources/ContainerResourceTest.java

+      out.write(ibm);
+    }
+    ContainerResource.Filesystem fs =
+        new ContainerResource.Filesystem() {


I think we usually add a constructor that accepts a supplier that can be replaced for testing, see

opentelemetry-java-instrumentation/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/HostIdResource.java

Line 56 in 60c7993

Supplier<String> getOsType,

Invert if statement to remove the need for negation Remove unneeded null check on return from charset.ForName Use a supplier constructor argument for testing

laurit · 2025-04-24T07:00:14Z

...rces/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java

+
+      // As a fallback value, add a hardcoded reference to IBM1047 (Canonical name: Cp1047).
+      Charset ibm1047 = Charset.forName("Cp1047");
+      if (ibm1047 != null) { // safety check


this can't be null either

…acks

benjamin-confino · 2025-04-27T20:37:03Z

I ran it through our zos test environment and it passed so I've pushed, I left it in a separate commit for now for ease of reviewing.

benjamin-confino · 2025-04-29T08:34:27Z

I see this is merged, thank you.

benjamin-confino requested a review from a team as a code owner April 17, 2025 16:39

benjamin-confino force-pushed the zos-charset-in-container-resource-fileset branch from 86d86fd to 2530c47 Compare April 17, 2025 17:05

benjamin-confino and others added 3 commits April 17, 2025 19:04

add method to read files on zos with correct encoding

f9672ef

add test to cover alternate encodings

a85fab6

update testAlternateEncoding to fit new implementation

e71b5a4

benjamin-confino force-pushed the zos-charset-in-container-resource-fileset branch from 2530c47 to e71b5a4 Compare April 17, 2025 18:04

laurit reviewed Apr 18, 2025

View reviewed changes

Make changes requested by code review:

2fec2e6

Invert if statement to remove the need for negation Remove unneeded null check on return from charset.ForName Use a supplier constructor argument for testing

laurit reviewed Apr 24, 2025

View reviewed changes

Simplify zos routine for selecting charset by removing unneeded fallb…

1435f3e

…acks

fix typo

ee6f9ff

laurit approved these changes Apr 28, 2025

View reviewed changes

trask merged commit 7364927 into open-telemetry:main Apr 28, 2025
86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable Agent Detection on Zos #13730

Re-enable Agent Detection on Zos #13730

benjamin-confino commented Apr 17, 2025

linux-foundation-easycla bot commented Apr 17, 2025 •

edited

Loading

benjamin-confino commented Apr 17, 2025

laurit Apr 18, 2025

benjamin-confino Apr 22, 2025

laurit Apr 18, 2025

benjamin-confino Apr 22, 2025

laurit Apr 24, 2025

laurit Apr 18, 2025

benjamin-confino Apr 22, 2025

laurit Apr 24, 2025 •

edited

Loading

benjamin-confino Apr 24, 2025

laurit Apr 25, 2025

benjamin-confino Apr 25, 2025

benjamin-confino Apr 27, 2025

laurit Apr 18, 2025

benjamin-confino Apr 22, 2025

laurit Apr 24, 2025

benjamin-confino commented Apr 27, 2025

benjamin-confino commented Apr 29, 2025

Re-enable Agent Detection on Zos #13730

Re-enable Agent Detection on Zos #13730

Conversation

benjamin-confino commented Apr 17, 2025

linux-foundation-easycla bot commented Apr 17, 2025 • edited Loading

benjamin-confino commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamin-confino commented Apr 27, 2025

benjamin-confino commented Apr 29, 2025

linux-foundation-easycla bot commented Apr 17, 2025 •

edited

Loading

laurit Apr 24, 2025 •

edited

Loading