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

Jump to bottom

fix(basic-tracer): add timestamp for event #195

Merged
merged 3 commits into from
Aug 14, 2019
Merged

fix(basic-tracer): add timestamp for event #195

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f03134c
fix(basic-tracer): add timestamp for event
mayurkale22 Aug 12, 2019
3cd6942
fix: use performance.now()
mayurkale22 Aug 12, 2019
c5c1028
Merge branch 'master' into timed_event
mayurkale22 Aug 14, 2019
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
4 changes: 2 additions & 2 deletions packages/opentelemetry-basic-tracer/src/Span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class Span implements types.Span, ReadableSpan {
readonly parentSpanId?: string;
readonly attributes: types.Attributes = {};
readonly links: types.Link[] = [];
readonly events: types.Event[] = [];
readonly events: types.TimedEvent[] = [];
readonly startTime: number;
name: string;
status: types.Status = {
Expand Down Expand Up @@ -79,7 +79,7 @@ export class Span implements types.Span, ReadableSpan {

addEvent(name: string, attributes?: types.Attributes): this {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also make an optional time parameter for the event?

That will be helpful for bringing over the OpenCensus Web code since it generates timed events based on browser performance data (this is similar to my desire to make the start span take a start time and end span take an end time, etc.)

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 not defined in the specs: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#add-events. AFAIK SDK is responsible for adding timestamp internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, OK. Should I raise an issue with the specs for this? What's the process for that?

I do need the feature for web tracing.

Copy link
Member Author

Choose a reason for hiding this comment

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

so the event timestamp is gonna be different from current timestamp (which we are adding internally) in web tracing?

What's the process for that?

Create new issue here and start the discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the pointer! I've created open-telemetry/opentelemetry-specification#213 to discuss and shared more about the web tracing use case there (basically generating timed events from Performance API data as measured by the browser runtime rather than in the JS code)

Copy link
Member

Choose a reason for hiding this comment

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

Oh huh okay I think that part of the spandata removal is going to have to include optional timestamps for events, because how else would you do out of band reporting without the timestamp? I'll comment on that issue @draffensperger

Copy link
Member Author

@mayurkale22 mayurkale22 Aug 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the issue @draffensperger. If all goes well, I will open new PR to merge TimedEvent and Event interface and add optional timestamp param to addEvent method. To unblock #192 work I'd like to merge this one. /cc @hekike

if (this._isSpanEnded()) return this;
this.events.push({ name, attributes });
this.events.push({ name, attributes, time: performance.now() });
return this;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-basic-tracer/src/export/ReadableSpan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import {
Status,
Attributes,
Link,
Event,
SpanContext,
TimedEvent,
} from '@opentelemetry/types';

export interface ReadableSpan {
Expand All @@ -33,5 +33,5 @@ export interface ReadableSpan {
readonly status: Status;
readonly attributes: Attributes;
readonly links: Link[];
readonly events: Event[];
readonly events: TimedEvent[];
}
35 changes: 15 additions & 20 deletions packages/opentelemetry-basic-tracer/test/Span.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,30 +176,25 @@ describe('Span', () => {
span.addEvent('sent');
let readableSpan = span.toReadableSpan();
assert.strictEqual(readableSpan.events.length, 1);
assert.deepStrictEqual(readableSpan.events, [
{
attributes: undefined,
name: 'sent',
},
]);
const [event] = readableSpan.events;
assert.deepStrictEqual(event.name, 'sent');
assert.ok(!event.attributes);
assert.ok(event.time > 0);

span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true });
readableSpan = span.toReadableSpan();
assert.strictEqual(readableSpan.events.length, 2);
assert.deepStrictEqual(readableSpan.events, [
{
attributes: undefined,
name: 'sent',
},
{
attributes: {
attr1: 'value',
attr2: 123,
attr3: true,
},
name: 'rev',
},
]);
const [event1, event2] = readableSpan.events;
assert.deepStrictEqual(event1.name, 'sent');
assert.ok(!event1.attributes);
assert.ok(event1.time > 0);
assert.deepStrictEqual(event2.name, 'rev');
assert.deepStrictEqual(event2.attributes, {
attr1: 'value',
attr2: 123,
attr3: true,
});
assert.ok(event2.time > 0);

span.end();
// shouldn't add new event
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-types/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export * from './trace/SpanOptions';
export * from './trace/span_context';
export * from './trace/span_kind';
export * from './trace/status';
export * from './trace/TimedEvent';
export * from './trace/tracer';
export * from './trace/trace_options';
export * from './trace/trace_state';
export * from './trace/tracer';