Skip to content

Conversation

@jpinkney-aws
Copy link
Contributor

Problem

  • Theres quite a few missues of telemetry.foo.record inside of the codebase and it can be confusing for external contributors

Solution

  • Clean up the telemetry interface by only allowing record/increment on spans

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jpinkney-aws jpinkney-aws requested a review from a team as a code owner September 23, 2024 13:53
@jpinkney-aws jpinkney-aws force-pushed the jpinkney-aws/simplify-telemetry-interface branch from 875b3f6 to f95e2f4 Compare September 25, 2024 18:49

// This function is expected to be called in the context of restoreConnection()
telemetry.auth_modifyConnection.record({
span.record({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for reference: this could also do telemetry.record() , right? In general, I don't think we can (or want to) except spans to be passed around; that defeats much of the advantage of the "execution scope" concept, which allows the context to be augmented by any codepath.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes, but it also has the side effect that every subsequent span thats created would have connectionState and authStatus

Instead I could have technically used telemetry.activeSpan?.record()

Comment on lines 25 to 31
interface Metric<T extends MetricBase = MetricBase> {
run<U>(fn: (span: Span<T>) => U, options?: SpanOptions): U
}

interface Span<T extends MetricBase = MetricBase> {
increment(data: { [P in NumericKeys<T>]+?: number }): void
run<U>(fn: (span: this) => U, options?: SpanOptions): U
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't span have run()? I don't quite understand the distinction here. I think of metrics as mostly synonymous with spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is this span as any is a lie. It never actually had run. When you try and do span.run you get "span.run is not a function"

The passed in span is just a telemetry span that has emit, record, increment but isn't a metric type

Problem:
- Theres quite a few missues of telemetry.foo.record inside of the codebase and it can be confusing for external contributors

Solution:
- Clean up the telemetry interface by only allowing record/increment on spans
@jpinkney-aws jpinkney-aws force-pushed the jpinkney-aws/simplify-telemetry-interface branch from f95e2f4 to ecca20d Compare October 4, 2024 00:24
@jpinkney-aws jpinkney-aws merged commit a386cc3 into master Oct 4, 2024
17 of 18 checks passed
@jpinkney-aws jpinkney-aws deleted the jpinkney-aws/simplify-telemetry-interface branch October 4, 2024 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants