[core] - Scope down OpenTelemetry interfaces #17730

maorleger · 2021-09-17T18:40:46Z

What

Remove as much of OpenTelemetry surface area as possible from core-tracing
- Removing the wrapper functions where we can
- Narrowing the OTel redeclared interfaces where possible
Move any usage of wrapper functions in tests to the test-utils package

Why

While being able to avoid re-declaring OTel's interfaces is what we really want, we're a long ways away from it. I do believe that
we can simplify our APIs gradually. My end-goal is to have every library use a (not-yet-implemented) higher level wrappers to
add tracing to methods without worrying about OTel at all.

But we do have a lot of debt here that makes this problematic. createSpan moved us in the right direction though and allows
us to take out anything that isn't used in product code.

For the interfaces we do leave behind one thing we can do is simplify them as much as possible by narrowing the input types

Callouts

There is an issue with Event Hubs and Service Bus that I need to address here - they do use some of these wrapper methods
in product code and upgrading their core-tracing would require some changes. I spoke to @chradek and we feel a good
compromise might be to have Event Hubs and Service Bus pull in @opentelemetry/api directly for their more advanced use cases.

While I'm not upgrading anyone yet, these changes make it so that when we do want to upgrade core-tracing the packages
will be ready to go.

check-enforcer · 2021-09-17T18:51:00Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

sdk/core/core-tracing/package.json

maorleger · 2021-09-17T20:21:27Z

sdk/test-utils/test-utils/src/index.ts

@@ -15,3 +15,4 @@ export { isNode, isNode8 } from "./utils";
 export { TestSpan } from "./tracing/testSpan";
 export * from "./tracing/testTracer";
 export * from "./tracing/testTracerProvider";
+export { trace } from "@opentelemetry/api";


I wanted to call this out... right now I am just exporting this as trace but that's a really generic name and not very meaningful unless you're pulling it from @opentelemetry/api...

Options are:

Rename it to something more meaningful like openTelemetryTrace

Bundle everything under the openTelemetry namespace and just export that

Keep it as trace

Export the individual wrapper methods like we do in core-tracing today

The benefit of (3) is that it'll be easily findable but the downside of course is that trace is not a great name

import { trace } from "@azure/test-utils"...

before I go and make a ton of changes I figured I'll bring this up and see what folks think...

I'm leaning towards either 1 or 2 depending on if we think there will be other things we want to re-export from OT

maorleger · 2021-09-17T20:53:52Z

sdk/core/core-tracing/review/core-tracing.api.md

@@ -33,42 +33,9 @@ export interface CreateSpanFunctionArgs {
    packagePrefix: string;
 }

-// @public
-export type Exception = ExceptionWithCode | ExceptionWithMessage | ExceptionWithName | string;


This is really just Error | string - for our usecase we can just say Error

maorleger · 2021-09-17T20:54:32Z

sdk/core/core-tracing/review/core-tracing.api.md

 // @public
 export function extractSpanContextFromTraceParentHeader(traceParentHeader: string): SpanContext | undefined;

-// @public
-export function getSpan(context: Context): Span | undefined;


These are all used in test code only (but see my callout about EH / SB in summary) so I got rid of them and had the test code updated to pull in from @azure/test-utils

maorleger · 2021-09-17T20:55:26Z

sdk/core/core-tracing/review/core-tracing.api.md

@@ -160,9 +118,6 @@ export enum SpanStatusCode {
    UNSET = 0
 }

-// @public
-export type TimeInput = HrTime | number | Date;


Let's just call this Date - we don't need to support everything and can assume the caller can make any conversions neeeded. We can always expand later.

maorleger · 2021-09-17T20:57:50Z

Figured I'll get input from @xirzec / @bterlson / @chradek before taking this out of draft and adding PR noise

xirzec · 2021-09-17T22:00:01Z

sdk/core/core-tracing/review/core-tracing.api.md

-
-// @public
-export function setSpanContext(context: Context, spanContext: SpanContext): Context;
-
 // @public
 export interface Span {


I love to see the simplification!

Could you lay out what the next sequence of steps you think are? e.g. will Span eventually be a re-export of OT? Do we need to have some minimal interface for it ourselves?

Same with context - can we avoid having SDKs interact with context directly or even have to know about it?

Sure!

Assumption: EventHubs and ServiceBus are ok taking a direct dependency on @opentelemetry/api (I still have to get confirmation on that and will reach out offline)

I should note though that at the end of the day we can remove what we can but we are likely stuck with some of these interfaces for the time being... they've been here for a while, packages have taken dependencies on them, and so we can't unfortunately pull the rug from under them anymore.

Must stay

SpanStatus, SpanStatusCode

isSpanContextValid

Span

SpanContext, TraceState

SpanOptions, Link, SpanKind, SpanAttributes, SpanAttribute

OperationTracingOptions + Context (to support manual span propagation)

I'd love to remove all of these, but these are the ones that will be really hard to tease out at this point

Still investigating if these can go

Tracer interface

ContextAPI

getTracer

context entrypoint

TraceFlags

Longer term (post GA)

I'd like to avoid having packages manage spans entirely using something like withTrace which is in early draft and likely will take time to migrate packages over.

I'd also like to re-export OTel interfaces directly but we will likely not get there by GA (there are a few things that have to happen first)

What do you think?

I'd also like to re-export OTel interfaces directly but we will likely not get there by GA (there are a few things that have to happen first)

Figured I'll expand what those are:

Figure out how to downlevel OTel's interfaces: can I even downlevel-dts interfaces I am re-exporting from OTel? Alternatively, can we set TS 3.8 as the min-bar for support? 😈

Upgrade API-extractor across the board to something that supports import/export type syntax (otherwise there will be a ton of build noise in the form of API Extractor warnings)

This is an interesting point. Since tracing is heavily dependent on OpenTelemetry, I think it's reasonable to match their support level for TS. So if they only work on TS 3.8+, I think it's fair that we are the same (for tracing interfaces, that is.) I don't want to be in the business of trying to monkeypatch their compat story.

Also I'm curious what the blocker is from re-exporting the OT shapes? Why does our definition of Span vs theirs make a meaningful difference to TS?

Also I'm curious what the blocker is from re-exporting the OT shapes? Why does our definition of Span vs theirs make a meaningful difference to TS?

There's two issues that I am aware of and I believe both are related to import type and export type being used in OTel's API

The first is that our version of API Extractor (7.7.x) does not support them so we end up with these warnings on every package when I try to re-export any APIs directly from OTel:

Warning: /home/mleger/workspace/worktrees/tracing/common/temp/node_modules/.pnpm/@[email protected]/node_modules/@opentelemetry/api/build/src/index.d.ts:27:8 - (TS2304) Cannot find name 'type'. Warning: /home/mleger/workspace/worktrees/tracing/common/temp/node_modules/.pnpm/@[email protected]/node_modules/@opentelemetry/api/build/src/index.d.ts:27:28 - (TS2304) Cannot find name 'from'. Warning: /home/mleger/workspace/worktrees/tracing/common/temp/node_modules/.pnpm/@[email protected]/node_modules/@opentelemetry/api/build/src/index.d.ts:31:8 - (TS2304) Cannot find name 'type'. Warning: /home/mleger/workspace/worktrees/tracing/common/temp/node_modules/.pnpm/@[email protected]/node_modules/@opentelemetry/api/build/src/index.d.ts:31:26 - (TS2304) Cannot find name 'from'.

See #9410 and #13557 as related issues (and of course upgrading API Extractor is not free because of how it handles name collision in later versions, see #17702 for an example)

The second is less clear to me - I'm hopeful @richardpark-msft or @chradek remember why #14618 was needed? That looks like where we removed the re-exports and redeclared the types

xirzec · 2021-09-17T22:02:49Z

sdk/test-utils/test-utils/src/index.ts

@@ -15,3 +15,4 @@ export { isNode, isNode8 } from "./utils";
 export { TestSpan } from "./tracing/testSpan";
 export * from "./tracing/testTracer";
 export * from "./tracing/testTracerProvider";
+export { trace } from "@opentelemetry/api";


I'm leaning towards either 1 or 2 depending on if we think there will be other things we want to re-export from OT

maorleger · 2021-09-20T15:27:40Z

sdk/core/core-tracing/src/interfaces.ts

- * @param context - context to use as parent
- * @param span - span to set active
- */
-export function setSpan(context: Context, span: Span): Context {


This (and setSpanContext) are being used in servicebus and eventhubs

I talked to @chradek about it, one option is they take a dependency on OpenTelemetry directly. Another is we keep these two functions here for the time being and GA with them

Mobile Network REST API Specification (Azure#17730) * Mobile Network REST API Specification * Removed API versions 2020-06-01-preview and 2021-04-01-preview. Have only the API version 2022-01-01-preview in Public * Addressed Swagger Avocado and Swagger SpellCheck Errors * Addressed Parameters Order Errors * Merge azure-rest-api-specs-pr branch ssivathas/mobilenetwork-public-api into satravi/mobilenetwork-rest-api-specs * Removed Unreferenced JSON Files * Added support for Static IP Addressing to PMN * Addressed Swagger PrettierCheck Errors * Addressed Swagger ModelValidation Errors * change go config to track 2 * Updated the API versions in SDK README files * Update readme.python.md * Update readme.md * Updated ActivationState to ConfigurationState * Addressed XmsIdentifierValidation Errors * Addressed DescriptionAndTitleMissing Errors * Generic name for N2 and N3 Interface attributes to support 4G Networks * Generic name for N6 Interface attribute to support 4G Networks * Generic name for N6 Interface attribute to support 4G Networks Co-authored-by: ArcturusZhang <[email protected]> Co-authored-by: Yuchao Yan <[email protected]>

ghost added Azure.Core Digital Twins Event Grid Event Hubs Service Bus Azure.Identity KeyVault Storage Storage Service (Queues, Blobs, Files) labels Sep 17, 2021

maorleger added 7 commits September 17, 2021 11:51

wip

61e1427

wip

92b05a2

wip

9eb638b

fix builds

35d018c

builds without helper functions

efdb0eb

put SpanAttributeValue back

ec84f87

restore SpanAttributeValue

1d47583

maorleger force-pushed the rm-error-interfaces branch from 9255265 to 1d47583 Compare September 17, 2021 18:54

a little bit of cleanup

b023fad

maorleger commented Sep 17, 2021

View reviewed changes

put core-tracing back to preview.14

7eedf67

maorleger commented Sep 17, 2021

View reviewed changes

maorleger requested review from xirzec, chradek and bterlson September 17, 2021 20:56

restore sb and eh tracing code

b5b5c84

xirzec reviewed Sep 17, 2021

View reviewed changes

maorleger commented Sep 20, 2021

View reviewed changes

maorleger closed this Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] - Scope down OpenTelemetry interfaces #17730

[core] - Scope down OpenTelemetry interfaces #17730

maorleger commented Sep 17, 2021 •

edited

Loading

check-enforcer bot commented Sep 17, 2021

maorleger Sep 17, 2021

xirzec Sep 17, 2021

maorleger Sep 17, 2021

maorleger Sep 17, 2021

maorleger Sep 17, 2021

maorleger commented Sep 17, 2021

xirzec Sep 17, 2021

maorleger Sep 17, 2021 •

edited

Loading

maorleger Sep 17, 2021 •

edited

Loading

xirzec Sep 20, 2021

xirzec Sep 20, 2021

maorleger Sep 20, 2021

xirzec Sep 17, 2021

maorleger Sep 20, 2021 •

edited

Loading

[core] - Scope down OpenTelemetry interfaces #17730

[core] - Scope down OpenTelemetry interfaces #17730

Conversation

maorleger commented Sep 17, 2021 • edited Loading

What

Why

Callouts

check-enforcer bot commented Sep 17, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

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

maorleger commented Sep 17, 2021

Choose a reason for hiding this comment

maorleger Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Must stay

Still investigating if these can go

Longer term (post GA)

maorleger Sep 17, 2021 • 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

maorleger Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

maorleger commented Sep 17, 2021 •

edited

Loading

maorleger Sep 17, 2021 •

edited

Loading

maorleger Sep 17, 2021 •

edited

Loading

maorleger Sep 20, 2021 •

edited

Loading