feat: Telemetry API #863

aajtodd · 2023-06-13T12:39:07Z

Issue #

N/A

Description of changes

Proposes a new unified interface for telemetry data. This interface aligns much closer to OpenTelemetry such that plugging in OTeL as the backing implementation should be easy (we may make tweaks along the way if we discover this not to be the case).

NOTE: This is just the interface, removal of previous tracing/logging and updating usage across the SDK will be in subsequent PRs.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ianbotsf · 2023-06-13T14:24:17Z

...ervability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/TelemetryProvider.kt

+    /**
+     * Get the [LoggerProvider] used to create new [aws.smithy.kotlin.runtime.telemetry.logging.LoggerProvider] instances
+     */
+    public val loggerProvider: LoggerProvider


Nit: used to create new aws.smithy.kotlin.runtime.telemetry.logging.Logger instances

ianbotsf · 2023-06-13T14:42:15Z

...ity/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/logging/LogRecordBuilder.kt

+/**
+ * Construct a logging record and emits it to an underlying logger
+ */
+public interface LogRecordBuilder {
+    /**
+     * Set the logging level (severity)
+     * @param level the level to log this record at
+     */
+    public fun setLevel(level: LogLevel)
+
+    /**
+     * Set the timestamp
+     * @param ts the observed time for this event
+     */
+    public fun setTimestamp(ts: Instant)
+
+    /**
+     * Set an exception associated with this event
+     * Some loggers will do additional formatting for exceptions.
+     * @param ex the exception to associate with this log record
+     */
+    public fun setCause(ex: Throwable)


Question: Values that can be written but not read feel unergonomic to me, as do explicit setters (although I understand that Kotlin does not yet support write-only properties). Is there a reason not to use full read/write vars here?

interface LogRecordBuilder { var level: LogLevel var timestamp: Instant var cause: Throwable ... }

I think the idea is to be no-ops in many cases (e.g. not being sampled, log level not enabled, etc).

ianbotsf · 2023-06-13T14:44:36Z

...ity/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/logging/LogRecordBuilder.kt

+    /**
+     * Associate all key/value pairs from [attributes] with this log record
+     * @param attributes the attributes to associate with this log record
+     */
+    public fun setAllAttributes(attributes: Attributes) {
+        attributes.keys.forEach { key ->
+            @Suppress("UNCHECKED_CAST")
+            setAttribute(key.name, attributes[key as AttributeKey<Any>])
+        }
+    }


Style: The name of this method seems misleading to me since it does not replace all the attributes (which is what "set" typically means). Maybe it "adds" or "merges" them?

Maybe just setAttributes then?

setAttributes also implies to me that all existing attributes would be replaced. In my preference, either we want that and the default impl changes here or we want to merge attributes and the method name changes to reflect that.

ianbotsf · 2023-06-13T14:47:54Z

...observability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/logging/Logger.kt

+    /**
+     * Lazy add a log message with throwable payload if trace logging is enabled
+     */
+    public fun trace(t: Throwable?, msg: () -> Any)


Question: Given that t is nullable here, is there a reason to offer an explicit overload without the Throwable argument? Could we have just one overload with a default value?

fun trace(t: Throwable? = null, msg: () -> Any)

We probably could yes. I think there may be some changes here still anyway FWIW since I need to figure out how we want to think about trace context and logs.

ianbotsf · 2023-06-13T15:10:08Z

...bility/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/metrics/MeterProvider.kt

+    /**
+     * Get or create a named [Meter]
+     * @param scope the name of the instrumentation scope that uniquely identifies this meter
+     * @param attributes instrumentation scope attributes to associate with emitted telemetry
+     */
+    public fun getMeter(scope: String, attributes: Attributes = emptyAttributes()): Meter


Style: The word "get" by itself typically implies a read-only operation but that's not what's described in the method docs. Kotlin's stdlib calls this sort of thing getOrPut but I've also seen getOrCreate.

We can change it (and tracer provider) to getOrCreate if you prefer.

ianbotsf · 2023-06-13T16:41:33Z

...bservability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/trace/TraceSpan.kt

+    /**
+     * Set all [attributes] on the span.
+     * @param attributes collection of attributes to set on the span
+     */
+    public fun setAttributes(attributes: Attributes)


Question: Does this mean "replace all existing attributes with the new attributes value"? Or merge?

ianbotsf · 2023-06-13T16:49:43Z

...bservability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/trace/TraceSpan.kt

+    /**
+     * Set an attribute on the span
+     * @param key the attribute key to use
+     * @param value the value to associate with the key
+     */
+    public fun <T : Any> setAttribute(key: AttributeKey<T>, value: T)


Style: Consider indexed getter/setter:

operator fun <T : Any> get(key: AttributeKey<T>): T? operator fun <T : Any> set(key: AttributeKey<T>, value: T)

You'll find most of the API is going to be write only and that is on purpose but we could change it to operator functions.

ianbotsf · 2023-06-13T16:50:25Z

...bservability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/trace/TraceSpan.kt

+    /**
+     * Set the span status
+     * @param status the status to set
+     */
+    public fun setStatus(status: SpanStatus)


Comment: As above, write but no read feels weird.

These are all write only in OTeL API (the SDK implementation allows reading attributes and other values in processors and exporters where they make sampling decisions, serialize, etc). If we made them readable we'd have to store the value in the shim layer(s) even though they may not be used. We don't really need to read any of this anywhere though.

ianbotsf · 2023-06-13T16:51:05Z

...bservability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/trace/TraceSpan.kt

+    /**
+     * Marks the end of this span's execution. This MUST be called when the unit
+     * of work the span represents has finished.
+     */
+    public fun end()


Question: If this MUST be called, should TraceSpan be Closeable and should end instead be close?

If we wanted to go that route yes it could be.

ianbotsf · 2023-06-13T16:52:16Z

...ability/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/trace/TracerProvider.kt

+    /**
+     * Returns a unique [Tracer] scoped to be used by instrumentation code. The scope
+     * and identity of that instrumentation code is uniquely identified by the name
+     * and attributes.
+     *
+     * @param scope the name of the instrumentation scope
+     * @param attributes (optional) specifies the instrumentation scope attributes to associate with emitted
+     * telemetry
+     */
+    public fun getTracer(scope: String, attributes: Attributes = emptyAttributes()): Tracer


Question: Is this also "get or create"?

sonarqubecloud · 2023-06-14T18:14:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
33 Code Smells

No Coverage information
0.0% Duplication

lauzadis · 2023-06-14T18:56:53Z

...ity/telemetry-api/common/src/aws/smithy/kotlin/runtime/telemetry/logging/LogRecordBuilder.kt

+import aws.smithy.kotlin.runtime.util.get
+
+/**
+ * Construct a logging record and emits it to an underlying logger


This builder doesn't emit to an underlying logger, it just constructs a logging record, right?

Correct, will fix in next PR.

aajtodd added 4 commits June 7, 2023 13:14

refactor: introduce new telemetry API for metrics and tracing

bb98dc9

add logging api

379cab1

remove trace context for opaque context

a18ea7a

dump api

04405e8

aajtodd requested a review from a team as a code owner June 13, 2023 12:39

ianbotsf reviewed Jun 13, 2023

View reviewed changes

aajtodd added 2 commits June 14, 2023 14:09

update API

a6b1b45

fix logger and api dump

0ed4482

ianbotsf approved these changes Jun 14, 2023

View reviewed changes

lauzadis approved these changes Jun 14, 2023

View reviewed changes

aajtodd merged commit 1c281fa into observability Jun 14, 2023

aajtodd deleted the telemetry-api branch June 14, 2023 19:14

aajtodd added a commit that referenced this pull request Jul 7, 2023

feat: Telemetry API (#863)

c222df3

aajtodd mentioned this pull request Jul 7, 2023

refactor!: observability API and OTeL provider #886

Merged

aajtodd added a commit that referenced this pull request Jul 17, 2023

feat: Telemetry API (#863)

e41c33d

feat: Telemetry API #863

feat: Telemetry API #863

Uh oh!

Conversation

aajtodd commented Jun 13, 2023

Issue #

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!