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.

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

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

Merged
merged 3 commits into from Aug 14, 2019

Conversation

@mayurkale22 mayurkale22 requested review from hekike and bg451 Aug 12, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 12, 2019

Codecov Report

Merging #195 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   93.59%   93.62%   +0.02%     
==========================================
  Files          50       50              
  Lines        1578     1585       +7     
  Branches       99       99              
==========================================
+ Hits         1477     1484       +7     
  Misses        101      101
Impacted Files Coverage Δ
packages/opentelemetry-basic-tracer/src/Span.ts 92.72% <100%> (ø) ⬆️
...kages/opentelemetry-basic-tracer/test/Span.test.ts 100% <100%> (ø) ⬆️
@@ -79,7 +79,7 @@ export class Span implements types.Span, ReadableSpan {

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

This comment has been minimized.

Copy link
@draffensperger

draffensperger Aug 12, 2019

Contributor

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.)

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Aug 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@draffensperger

draffensperger Aug 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Aug 13, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@draffensperger

draffensperger Aug 13, 2019

Contributor

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)

This comment has been minimized.

Copy link
@bg451

bg451 Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Aug 14, 2019

Author Contributor

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

@draffensperger
Copy link
Contributor

left a comment

LGTM on this given the current spec

@bg451
bg451 approved these changes Aug 13, 2019

@mayurkale22 mayurkale22 merged commit e114423 into open-telemetry:master Aug 14, 2019

8 checks passed

ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node11 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation mayurkale22 authorized
Details

@mayurkale22 mayurkale22 deleted the mayurkale22:timed_event branch Aug 14, 2019

@mayurkale22 mayurkale22 referenced this pull request Aug 15, 2019
@hekike

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.