Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] - Scope down OpenTelemetry interfaces #17730

Closed
wants to merge 10 commits into from
Closed

[core] - Scope down OpenTelemetry interfaces #17730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
61e1427
wip
maorleger Sep 14, 2021
92b05a2
wip
maorleger Sep 15, 2021
9eb638b
wip
maorleger Sep 15, 2021
35d018c
fix builds
maorleger Sep 15, 2021
efdb0eb
builds without helper functions
maorleger Sep 17, 2021
ec84f87
put SpanAttributeValue back
maorleger Sep 17, 2021
1d47583
restore SpanAttributeValue
maorleger Sep 17, 2021
b023fad
a little bit of cleanup
maorleger Sep 17, 2021
7eedf67
put core-tracing back to preview.14
maorleger Sep 17, 2021
b5b5c84
restore sb and eh tracing code
maorleger Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions sdk/core/core-http/test/policies/tracingPolicyTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ import {
SpanContext,
TraceFlags,
TraceState,
setSpan,
context,
SpanStatusCode,
SpanStatus,
SpanAttributes,
SpanOptions,
SpanAttributeValue
} from "@azure/core-tracing";
import { tracingPolicy } from "../../src/policies/tracingPolicy";
import { TracerProvider, Tracer, Span, SpanOptions, trace } from "@opentelemetry/api";
import { TracerProvider, Tracer, Span, trace } from "@opentelemetry/api";
import sinon from "sinon";

class MockSpan implements Span {
Expand Down Expand Up @@ -210,7 +210,7 @@ describe("tracingPolicy", function() {
mockTracerProvider.setTracer(mockTracer);

const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand All @@ -237,7 +237,7 @@ describe("tracingPolicy", function() {
mockTracerProvider.setTracer(mockTracer);

const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand Down Expand Up @@ -265,7 +265,7 @@ describe("tracingPolicy", function() {
const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED, mockTraceState);
mockTracerProvider.setTracer(mockTracer);
const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand Down Expand Up @@ -293,7 +293,7 @@ describe("tracingPolicy", function() {
const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED, mockTraceState);
mockTracerProvider.setTracer(mockTracer);
const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(
{
Expand Down Expand Up @@ -336,7 +336,7 @@ describe("tracingPolicy", function() {
it("will not set headers if span is a NoOpSpan", async () => {
mockTracerProvider.disable();
const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand All @@ -351,7 +351,7 @@ describe("tracingPolicy", function() {
mockTracerProvider.setTracer(mockTracer);

const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand All @@ -366,7 +366,7 @@ describe("tracingPolicy", function() {
mockTracerProvider.setTracer(errorTracer);

const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());

Expand All @@ -382,7 +382,7 @@ describe("tracingPolicy", function() {
sinon.stub(errorTracer, "startSpan").returns(errorSpan);

const request = new WebResource();
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());

Expand All @@ -398,10 +398,9 @@ describe("tracingPolicy", function() {
request.spanOptions = {
attributes: { "az.namespace": "value_from_span_options" }
};
request.tracingContext = setSpan(context.active(), ROOT_SPAN).setValue(
Symbol.for("az.namespace"),
"value_from_context"
);
request.tracingContext = trace
.setSpan(context.active(), ROOT_SPAN)
.setValue(Symbol.for("az.namespace"), "value_from_context");

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand All @@ -420,7 +419,7 @@ describe("tracingPolicy", function() {
request.spanOptions = {
attributes: { "az.namespace": "value_from_span_options" }
};
request.tracingContext = setSpan(context.active(), ROOT_SPAN);
request.tracingContext = trace.setSpan(context.active(), ROOT_SPAN);

const policy = tracingPolicy().create(mockPolicy, new RequestPolicyOptions());
await policy.sendRequest(request);
Expand Down
32 changes: 15 additions & 17 deletions sdk/core/core-rest-pipeline/test/tracingPolicy.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@ import {
TraceFlags,
TraceState,
context,
setSpan,
SpanStatus,
SpanStatusCode,
SpanAttributes,
SpanAttributeValue,
SpanOptions
SpanOptions,
SpanAttributeValue
} from "@azure/core-tracing";
import { TracerProvider, Tracer, Span, trace } from "@opentelemetry/api";

Expand Down Expand Up @@ -212,7 +211,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand Down Expand Up @@ -251,7 +250,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand Down Expand Up @@ -290,7 +289,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand Down Expand Up @@ -328,7 +327,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const policy = tracingPolicy();
Expand Down Expand Up @@ -366,7 +365,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand All @@ -391,7 +390,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand All @@ -416,7 +415,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand All @@ -443,7 +442,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand All @@ -469,10 +468,9 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN).setValue(
Symbol.for("az.namespace"),
"value_from_context"
)
tracingContext: trace
.setSpan(context.active(), ROOT_SPAN)
.setValue(Symbol.for("az.namespace"), "value_from_context")
}
});
Object.assign(request.tracingOptions, {
Expand Down Expand Up @@ -503,7 +501,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
Object.assign(request.tracingOptions, {
Expand Down Expand Up @@ -534,7 +532,7 @@ describe("tracingPolicy", function() {
const request = createPipelineRequest({
url: "https://bing.com",
tracingOptions: {
tracingContext: setSpan(context.active(), ROOT_SPAN)
tracingContext: trace.setSpan(context.active(), ROOT_SPAN)
}
});
const response: PipelineResponse = {
Expand Down
57 changes: 6 additions & 51 deletions sdk/core/core-tracing/review/core-tracing.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,42 +33,9 @@ export interface CreateSpanFunctionArgs {
packagePrefix: string;
}

// @public
export type Exception = ExceptionWithCode | ExceptionWithMessage | ExceptionWithName | string;
Copy link
Member Author

Choose a reason for hiding this comment

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

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


// @public
export interface ExceptionWithCode {
code: string | number;
message?: string;
name?: string;
stack?: string;
}

// @public
export interface ExceptionWithMessage {
code?: string | number;
message: string;
name?: string;
stack?: string;
}

// @public
export interface ExceptionWithName {
code?: string | number;
message?: string;
name: string;
stack?: string;
}

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

// @public
export function getSpan(context: Context): Span | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

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


// @public
export function getSpanContext(context: Context): SpanContext | undefined;

// @public
export function getTraceParentHeader(spanContext: SpanContext): string | undefined;

Expand All @@ -78,9 +45,6 @@ export function getTracer(): Tracer;
// @public
export function getTracer(name: string, version?: string): Tracer;

// @public
export type HrTime = [number, number];

// @public
export function isSpanContextValid(context: SpanContext): boolean;

Expand All @@ -95,18 +59,12 @@ export interface OperationTracingOptions {
tracingContext?: Context;
}

// @public
export function setSpan(context: Context, span: Span): Context;

// @public
export function setSpanContext(context: Context, spanContext: SpanContext): Context;

// @public
export interface Span {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@maorleger maorleger Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@maorleger maorleger Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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? 😈
  2. 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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

addEvent(name: string, attributesOrStartTime?: SpanAttributes | TimeInput, startTime?: TimeInput): this;
end(endTime?: TimeInput): void;
addEvent(name: string, attributesOrStartTime?: SpanAttributes | Date, startTime?: Date): this;
end(endTime?: Date): void;
isRecording(): boolean;
recordException(exception: Exception, time?: TimeInput): void;
recordException(exception: Error, time?: Date): void;
setAttribute(key: string, value: SpanAttributeValue): this;
setAttributes(attributes: SpanAttributes): this;
setStatus(status: SpanStatus): this;
Expand All @@ -116,11 +74,11 @@ export interface Span {

// @public
export interface SpanAttributes {
[attributeKey: string]: SpanAttributeValue | undefined;
[attributeKey: string]: SpanAttributeValue;
}

// @public
export type SpanAttributeValue = string | number | boolean | Array<null | undefined | string> | Array<null | undefined | number> | Array<null | undefined | boolean>;
export type SpanAttributeValue = string | number | boolean;

// @public
export interface SpanContext {
Expand All @@ -144,7 +102,7 @@ export interface SpanOptions {
attributes?: SpanAttributes;
kind?: SpanKind;
links?: Link[];
startTime?: TimeInput;
startTime?: Date;
}

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

// @public
export type TimeInput = HrTime | number | Date;
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// @public
export const enum TraceFlags {
NONE = 0,
Expand Down
3 changes: 1 addition & 2 deletions sdk/core/core-tracing/src/createSpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
OperationTracingOptions,
Span,
SpanOptions,
setSpan,
context as otContext,
getTracer,
Context,
Expand Down Expand Up @@ -199,7 +198,7 @@ export function createSpanFunction(args: CreateSpanFunctionArgs) {
const newTracingOptions = {
...tracingOptions,
spanOptions: newSpanOptions,
tracingContext: setSpan(tracingContext, span)
tracingContext: trace.setSpan(tracingContext, span)
};

const newOperationOptions = {
Expand Down
10 changes: 0 additions & 10 deletions sdk/core/core-tracing/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,10 @@ export {
context,
Context,
ContextAPI,
Exception,
ExceptionWithCode,
ExceptionWithMessage,
ExceptionWithName,
getSpan,
getSpanContext,
getTracer,
HrTime,
isSpanContextValid,
Link,
OperationTracingOptions,
setSpan,
setSpanContext,
Span,
SpanAttributes,
SpanAttributeValue,
Expand All @@ -30,7 +21,6 @@ export {
SpanOptions,
SpanStatus,
SpanStatusCode,
TimeInput,
TraceFlags,
Tracer,
TraceState
Expand Down
Loading