Basic reporting through toString #220

sherriesyt · 2023-05-18T04:02:43Z

This is super not ready, just opening an early draft for visibility. Meant for #188.

I think I realized after this first commit that since Otel4s is a trait, the implementation of toString shouldn't lie here (instead, it should just specify that a String is returned) -- is this correct? (And we leave the implementation to all the things with type Otel4s[F[_]] / all the things that extend Otel4s[F[_]]?)

iRevive · 2023-05-18T07:57:42Z

And we leave the implementation to all the things

You are correct. Currently, we have only one 'real' implementation here https://github.com/typelevel/otel4s/blob/main/java/all/src/main/scala/org/typelevel/otel4s/java/OtelJava.scala#L67.

We can override the toString right in this anonymous class.

The OpenTelemetry classes in Java, like io.opentelemetry.sdk.OpenTelemetrySdk, provide sufficient information in their toString implementation. We have two options: either delegate to jOtel.toString() or incorporate it into our own custom message.

For now, it seems like delegating should be enough.

sherriesyt · 2023-05-18T23:54:56Z

So, I sat in a call with @armanbilge and wrote one test case to observe the output of JOpenTelemetry's noop implementation's toString method, and it's...not pretty.

8:   test("toString implementation") {
9:     assertEquals(JOpenTelemetry.noop().toString(), "noop?")
10:  }
values are not the same
=> Obtained
io.opentelemetry.api.DefaultOpenTelemetry@4299b15b
=> Diff (- obtained, + expected)
-io.opentelemetry.api.DefaultOpenTelemetry@4299b15b
+noop?

Wanted to get some opinions on how to proceed.

armanbilge · 2023-05-18T23:56:37Z

java/all/src/main/scala/org/typelevel/otel4s/java/OtelJava.scala

@@ -68,6 +68,8 @@ object OtelJava {
 def propagators: ContextPropagators[F] = contextPropagators
 def meterProvider: MeterProvider[F] = metrics.meterProvider
 def tracerProvider: TracerProvider[F] = traces.tracerProvider
+
+ override def toString: String = jOtel.toString()


I wonder if the Java OpenTelemetry#toString is not guaranteed to be nice in general, but we can still use propgators.toString, meterProvider.toString, tracerProvider.toString etc. to put together our own toString?

The uninformative .toString you're getting is the default from java.lang.Object.

In general, the Java Otel API is built on interfaces. I wrote 99% of my Java before interfaces could have default implementations, and I don't know whether an interface can override java.lang.Object#toString. Legal or not, I can confirm some, including OpenTelemetry, don't. "Not guaranteed to be nice" is correct.

Some implementations have a .toString, like DefaultContextPropagators. Some do not, like DefaultTracerProvider. So you'd make some progress going down that road, but won't be rid of the hexadecimal hashCodes, and ... there can be multiple implementations of these interfaces. See "no guarantees."

The golden rule of software quality tells us to upstream better .toStrings to Java, but that's not what @sherriesyt volunteered for. If that's interesting anyway, I can help. If it's not... I've been writing Java for a quarter century, and another hour probably wouldn't kill me.

I don't mind dabbling in Java (though I'm rusty) -- can you point me to where to start? Is this somewhere in opentelemetry-java?

Yes. The noop value in your test is implemented here, and a .toString in that class would make it nicer.

The one I would expect to see in production is here, and already has a thorough toString. So while the easy one to test is in rough shape, the good news is the common production one should already work pretty well!

If you decide to chase this upstream, the build tool is Gradle. I've used gradle publishToMavenLocal before. You can see the version number it generates in the output (I don't have it handy to be exact), and you can put that version into build.sbt to see how your work interacts with Otel4s.

rossabaker · 2023-05-23T03:33:36Z

There are a lot of checks for formatting and licenses and what not. There's a prePR command added to SBT that runs them all. I'm not in the habit of running it myself, but it's a way to find them locally instead of one-by-one in GitHub Actions.

scalafmtSbt should fix the most recent build failure. You'd think scalafmtAll formats all, but it doesn't format SBT. 🤷

sherriesyt · 2023-05-23T03:35:14Z

Ah, you got around to commenting first, I pushed and in the midst of writing up a comment here my stomach went "I ache now."¹

I did the lazy² thing and tried out the other toString method you linked. It looks substantially more informative, though maybe a little visually messy?

One small question out of this is that I don't have a clean way of asserting that the output equals some expected string, because (e.g.) NoopLogRecordProcessor@{insert-hex-here} (that is part of the toString output) will always have some different hex code. Any advice on what assertion I can make in this test case?

One bigger question: sorry, you slightly lost me at Gradle. Would I be building the Java SDK (cloning their repo and so forth) using Gradle (presumably with my tiny local change) and then feeding the version of the thing I built into the build.sbt of Otel4s? (and why gradle publishToMavenLocal and not use Maven? Just a noob question of mine, I only have some experience with Maven and none with Gradle)

_{[edit: footnotes]

1: all fine now, I was happy with my dinner tonight, my stomach less so

2: lazy because I didn't fix the upstream problem first}

iRevive · 2023-05-23T07:30:26Z

Until the upstream 'toString' of noop SDK is addressed, can we detect the noop instance by comparing it with OpenTelemetry.noop()?

For example:

val isNoop = jOtel == JOpenTelemetry.noop()

new Otel4s { 
  def toString = if (isNoop) "Noop - and something informative there if we want :)" else jOtel.toString()
}

One small question out of this is that I don't have a clean way of asserting that the output equals some expected string

We can dynamically inject value for the telemetry.sdk.version attribute. We use BuildInfo sbt plugin to collect build details during compilation.

And that's how we configure it https://github.com/typelevel/otel4s/blob/main/build.sbt#L163-L167. Currently, we collect only openTelemetrySdkVersion, and that's exactly what we need.

Then, you can inject BuildInfo.openTelemetrySdkVersion into the template string:

s"OpenTelemetrySdk{... telemetry.sdk.version=\"${BuildInfo.openTelemetrySdkVersion}\"}} ..."

We do a similar trick in a different test - https://github.com/typelevel/otel4s/blob/main/java/metrics/src/test/scala/org/typelevel/otel4s/java/metrics/CounterSuite.scala#L56

NoopLogRecordProcessor@{insert-hex-here}

This one is more complicated. As Ross suggested, we can contribute a meaningful toString of the NoopLogRecordProcessor upstream.

For example, NoopTextMapPropagator has a custom toString, while NoopLogRecordProcessor does not.

armanbilge

One small question out of this is that I don't have a clean way of asserting that the output equals some expected string, because (e.g.) NoopLogRecordProcessor@{insert-hex-here} (that is part of the toString output) will always have some different hex code. Any advice on what assertion I can make in this test case?

Instead of checking for exact equality, you can do something like:

val got = (...).toString
assert(clue(got).startsWith("NoopLogRecordProcessor"))

(Anything wrapped in clue(...) will be printed out if the assert fails, to give a "clue" as to what went wrong.)

One bigger question: sorry, you slightly lost me at Gradle. Would I be building the Java SDK (cloning their repo and so forth) using Gradle (presumably with my tiny local change) and then feeding the version of the thing I built into the build.sbt of Otel4s?

Yup exactly, this is what Ross was getting at. Personally I don't think it's worth the fuss (sorry Ross ;) since it's hard to go wrong with a toString, so there's little to gain by further testing it in our own codebase. But in theory it shouldn't be too hard to do.

(and why gradle publishToMavenLocal and not use Maven? Just a noob question of mine, I only have some experience with Maven and none with Gradle)

Hmm, "Maven" is a bit over-loaded. It is actually 2 (or 3 ...) things:

a build tool
a repository format for publishing artifacts
Maven Central, a public repository where effectively the entire Java ecosystem publishes/retrieves artifacts

In this case, Gradle is the build tool, and publishToMavenLocal publishes it to your computer's local, Maven-formatted repository. You can't use the Maven build tool, because the Java OTel SDK repository is using the Gradle build tool (similarly, we are using the sbt build tool instead of the Maven build tool).

armanbilge · 2023-05-25T13:47:39Z

java/all/src/test/scala/org/typelevel/otel4s/java/OtelJavaSuite.scala

+ test("JOtelSDK toString returns useful info") {
+ assertEquals(
+ JOpenTelemetrySdk.builder().build().toString(),
+ "OpenTelemetrySdk{tracerProvider=SdkTracerProvider{clock=SystemClock{}, idGenerator=RandomIdGenerator{}, resource=Resource{schemaUrl=null, attributes={service.name=\"unknown_service:java\", telemetry.sdk.language=\"java\", telemetry.sdk.name=\"opentelemetry\", telemetry.sdk.version=\"1.26.0\"}}, spanLimitsSupplier=SpanLimitsValue{maxNumberOfAttributes=128, maxNumberOfEvents=128, maxNumberOfLinks=128, maxNumberOfAttributesPerEvent=128, maxNumberOfAttributesPerLink=128, maxAttributeValueLength=2147483647}, sampler=ParentBased{root:AlwaysOnSampler,remoteParentSampled:AlwaysOnSampler,remoteParentNotSampled:AlwaysOffSampler,localParentSampled:AlwaysOnSampler,localParentNotSampled:AlwaysOffSampler}, spanProcessor=NoopSpanProcessor{}}, meterProvider=SdkMeterProvider{clock=SystemClock{}, resource=Resource{schemaUrl=null, attributes={service.name=\"unknown_service:java\", telemetry.sdk.language=\"java\", telemetry.sdk.name=\"opentelemetry\", telemetry.sdk.version=\"1.26.0\"}}, metricReaders=[], views=[]}, loggerProvider=SdkLoggerProvider{clock=SystemClock{}, resource=Resource{schemaUrl=null, attributes={service.name=\"unknown_service:java\", telemetry.sdk.language=\"java\", telemetry.sdk.name=\"opentelemetry\", telemetry.sdk.version=\"1.26.0\"}}, logLimits=LogLimits{maxNumberOfAttributes=128, maxAttributeValueLength=2147483647}, logRecordProcessor=io.opentelemetry.sdk.logs.NoopLogRecordProcessor@59364403}, propagators=DefaultContextPropagators{textMapPropagator=NoopTextMapPropagator}}"


Well, this is something 😂

As a next step, can we adjust the test to build an Otel4s[IO] with the JOpenTelemetrySdk and check its toString? Currently this test is not exercising otel4s code at all 😛

armanbilge

🎉 lgtm!

iRevive · 2023-05-31T18:17:02Z

Eventually, we will get a better toString from the upstream: open-telemetry/opentelemetry-java#5493.

iRevive · 2023-05-31T18:17:11Z

I'm happy with the current approach, thanks @sherriesyt!

sherriesyt · 2023-06-01T02:17:57Z

Thanks @iRevive for the feedback & for improving the Java toString! :)

rossabaker · 2023-06-01T18:17:16Z

Personally I don't think it's worth the fuss (sorry Ross ;) since it's hard to go wrong with a toString, so there's little to gain by further testing it in our own codebase. But in theory it shouldn't be too hard to do.

Yeah, I don't want to overspecify the upstream library's toString in our own tests. My golden rule comment was wanting to make sure that any improvements we make happen upstream, like the one Maksym linked. I think we're all good here.

Basic reporting through toString

Override Otel4s toString with dummy message

88e3e0e

armanbilge linked an issue May 18, 2023 that may be closed by this pull request

Basic reporting of the OpenTelemetry configuration #188

Closed

Write test case to observe Java SDK toString output

a83402a

armanbilge reviewed May 18, 2023

View reviewed changes

Use JOTel SDK's toString in test case instead

6d32400

armanbilge reviewed May 25, 2023

View reviewed changes

sherriesyt added 3 commits May 30, 2023 00:15

Pull from main, merge

27bac8b

In progress: use JavaSDK to create Otel4s test

b088d96

Fix OtelJava toString test

a0b9022

sherriesyt marked this pull request as ready for review May 31, 2023 01:35

armanbilge approved these changes May 31, 2023

View reviewed changes

iRevive approved these changes May 31, 2023

View reviewed changes

armanbilge merged commit b7e17ff into typelevel:main Jun 1, 2023

sherriesyt deleted the issue-188 branch June 4, 2023 05:04

chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023

Merge pull request typelevel#220 from sherriesyt/issue-188

674d688

Basic reporting through toString

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic reporting through toString #220

Basic reporting through toString #220

sherriesyt commented May 18, 2023

iRevive commented May 18, 2023 •

edited

Loading

sherriesyt commented May 18, 2023

armanbilge May 18, 2023

rossabaker May 19, 2023

sherriesyt May 19, 2023

rossabaker May 19, 2023

rossabaker commented May 23, 2023

sherriesyt commented May 23, 2023 •

edited

Loading

iRevive commented May 23, 2023 •

edited

Loading

armanbilge left a comment

armanbilge May 25, 2023

armanbilge left a comment

iRevive commented May 31, 2023

iRevive commented May 31, 2023

sherriesyt commented Jun 1, 2023

rossabaker commented Jun 1, 2023

Basic reporting through toString #220

Basic reporting through toString #220

Conversation

sherriesyt commented May 18, 2023

iRevive commented May 18, 2023 • edited Loading

sherriesyt commented May 18, 2023

armanbilge May 18, 2023

Choose a reason for hiding this comment

rossabaker May 19, 2023

Choose a reason for hiding this comment

sherriesyt May 19, 2023

Choose a reason for hiding this comment

rossabaker May 19, 2023

Choose a reason for hiding this comment

rossabaker commented May 23, 2023

sherriesyt commented May 23, 2023 • edited Loading

iRevive commented May 23, 2023 • edited Loading

armanbilge left a comment

Choose a reason for hiding this comment

armanbilge May 25, 2023

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

iRevive commented May 31, 2023

iRevive commented May 31, 2023

sherriesyt commented Jun 1, 2023

rossabaker commented Jun 1, 2023

iRevive commented May 18, 2023 •

edited

Loading

sherriesyt commented May 23, 2023 •

edited

Loading

iRevive commented May 23, 2023 •

edited

Loading